test(runtime): cover expired directory lease cleanup - #10960
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
A new fire-and-forget Task from periodic cleanup is discarded without .Ignore(), risking unobserved exceptions and deviating from established runtime patterns.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Lite
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
 |
src/Orleans.Runtime/GrainDirectory/DistributedGrainDirectory.cs — CleanupExpiredLeases() now returns a Task, but this periodic loop discards it using _ = ....… |
What changed in this PR
Adds deterministic, fake-time coverage for GrainDirectoryPartition.CleanupExpiredLeasesCore by introducing a test hook which can trigger queued cleanup and return aggregate counts, enabling assertions around exact-boundary expiration, selective removal, and idempotence without sleeps/polling.
Changes:
- Add a 3-silo fake-time test scenario which exercises cleanup before expiration, at exact expiration, and repeated cleanup without time advancing.
- Extend
IGrainDirectoryTestHookswithCleanupExpiredLeasesAsync()and introduceGrainDirectoryLeaseCleanupResultto return cleanup counts. - Update directory partition cleanup entry point to queue cleanup and capture counts, and adjust the periodic cleanup caller accordingly.
| File | Description |
|---|---|
| test/Orleans.GrainDirectory.Tests/GrainDirectory/GrainDirectoryLeaseTests.cs | Adds deterministic, fake-time test coverage for expired lease cleanup behavior and idempotence across partitions. |
| src/Orleans.Runtime/OrleansContracts.txt | Updates generated contract declarations to include CleanupExpiredLeasesAsync on IGrainDirectoryTestHooks. |
| src/Orleans.Runtime/GrainDirectory/IGrainDirectoryPartition.cs | Adds CleanupExpiredLeasesAsync test hook method and the GrainDirectoryLeaseCleanupResult serialized return type. |
| src/Orleans.Runtime/GrainDirectory/GrainDirectoryPartition.cs | Implements queued cleanup result capture used by the new test hook while reusing CleanupExpiredLeasesCore. |
| src/Orleans.Runtime/GrainDirectory/DistributedGrainDirectory.cs | Adjusts periodic cleanup loop to call the new CleanupExpiredLeases() Task-returning method. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@copilot review |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The production periodic cleanup path now allocates and computes a cleanup result which is discarded, introducing avoidable overhead in a runtime hot path.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Lite
Findings: 1
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
|
src/Orleans.Runtime/GrainDirectory/GrainDirectoryPartition.cs — CleanupExpiredLeases() is now used by the production periodic cleanup loop, but it always… |
 |
src/Orleans.Runtime/GrainDirectory/DistributedGrainDirectory.cs — If CleanupExpiredLeases() is restored to a fire-and-forget void method to avoid per-tick result… |
Issues resolved since last review (1)
| Severity | Finding |
|---|---|
|
src/Orleans.Runtime/GrainDirectory/DistributedGrainDirectory.cs — CleanupExpiredLeases() now returns a Task, but this periodic loop discards it using _ = ....… View resolved comment |
|
@copilot review |
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
The new test currently relies on a magic-duration calculation and duplicated cluster setup which makes it brittle and harder to maintain as membership/lease settings evolve.
Review tier: Lite
Findings: None
Issues resolved since last review (2)
| Severity | Finding |
|---|---|
|
src/Orleans.Runtime/GrainDirectory/DistributedGrainDirectory.cs — If CleanupExpiredLeases() is restored to a fire-and-forget void method to avoid per-tick result… View resolved comment |
|
src/Orleans.Runtime/GrainDirectory/GrainDirectoryPartition.cs — CleanupExpiredLeases() is now used by the production periodic cleanup loop, but it always… View resolved comment |
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
test/Orleans.GrainDirectory.Tests/GrainDirectory/GrainDirectoryLeaseTests.cs:568
- The test hard-codes the effective dead-silo lease duration as
directoryLeaseDuration - 1s, which is implicitly tied to the configured membership failure-detection timeout. This is brittle: ifClusterMembershipOptionsdefaults or the test’s configured probe settings change, the expected lease expiration will drift and the test will fail for non-behavioral reasons. Compute the expected lease duration using the same production helper used byDistributedGrainDirectoryinstead of subtracting a magic constant.
This issue also appears on line 761 of the same file.
test/Orleans.GrainDirectory.Tests/GrainDirectory/GrainDirectoryLeaseTests.cs:782
CreateCleanupClusterduplicates the configuration logic inCreateCluster(same distributed directory enablement, membership manager wiring, time provider registration, and membership options). This increases the chance these helpers diverge over time and makes failures harder to diagnose when one helper is updated but the other is not. Consider consolidating into a single helper which takessiloCountandrangeLeaseDurationparameters so all lease tests share one canonical cluster configuration.
private static (InProcessTestCluster Cluster, FakeTimeProvider TimeProvider) CreateCleanupCluster(
TimeSpan rangeLeaseDuration)
{
var timeProvider = new FakeTimeProvider(InitialTime);
var builder = new InProcessTestClusterBuilder(3);
#pragma warning disable ORLEANSEXP003
builder.Options.UseDistributedGrainDirectory = true;
#pragma warning restore ORLEANSEXP003
builder.ConfigureSilo((_, siloBuilder) =>
{
siloBuilder.Services.AddSingleton<MembershipTableManager>();
siloBuilder.Services.AddSingleton<IMembershipManager, LeaseTestMembershipManager>();
siloBuilder.Services.AddSingleton<TimeProvider>(timeProvider);
siloBuilder.Services.AddKeyedSingleton(TimeProviderNames.Membership, TimeProvider.System);
siloBuilder.Services.Configure<ClusterMembershipOptions>(options =>
{
options.ProbeTimeout = TimeSpan.FromSeconds(1);
options.MaxProbeTimeout = TimeSpan.FromSeconds(1);
options.NumMissedProbesLimit = 1;
});
siloBuilder.Services.PostConfigure<GrainDirectoryOptions>(options => options.RangeLeaseDuration = rangeLeaseDuration);
});
|
@copilot review |
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The changes are additive and test-focused, with the runtime impact isolated to an internal test hook and no verified correctness, compatibility, or operational issues found in the updated code paths.
Review tier: Lite
Findings: None
Summary
GrainDirectoryPartition.CleanupExpiredLeasesCorewas the highest-risk remaining directory hotspot at 0% line coverage, 0% branch coverage, complexity 20, and CRAP 420. Lease cleanup had no deterministic test which proved exact-boundary expiration, selective registration removal, or idempotence.This change adds a three-silo fake-time scenario which creates two lease generations, invokes cleanup before expiration, at the first exact expiration, and again without advancing time. It asserts that cleanup retains active holds, prunes expired range and silo holds, removes only registrations on the expired silo, preserves later and live state, and produces the expected registration and lookup outcomes without sleeps or polling.
An internal
IGrainDirectoryTestHooksoperation provides an awaited scheduler barrier and six aggregate cleanup counts. It routes through the production queued entry point, exposes no private identities, and leaves periodic scheduling unchanged.Focused coverage moves
CleanupExpiredLeasesCorefrom 0% line / 0% branch / CRAP 420 to 91.5% line / 100% branch / CRAP 20.25.GrainDirectoryPartition.csmoves from 63.7% to 74.9% line coverage and from 62.7% to 74.5% branch coverage in the focused lease fixture.Contributes to #10858.
Microsoft Reviewers: Open in CodeFlow