Skip to content

Conversation

@sophiatev
Copy link
Contributor

This PR introduces the extended sessions feature for entities in .NET isolated. It essentially copies what was done to enable extended sessions for orchestrations in the GrpcOrchestrationRunner into the GrpcEntityRunner, and introduces a new class for shared logic between the two.

Copilot AI review requested due to automatic review settings November 20, 2025 00:28
Copilot finished reviewing on behalf of sophiatev November 20, 2025 00:32
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR extends the extended sessions feature to entities in .NET isolated, bringing entities to feature parity with orchestrations. The implementation refactors shared logic between GrpcOrchestrationRunner and GrpcEntityRunner into a new utility class and applies the same extended session caching pattern to entities.

Key Changes:

  • Introduced GrpcInstanceRunnerUtils to share request property parsing and cache initialization logic
  • Added extended sessions support to GrpcEntityRunner with state caching capabilities
  • Updated parameter naming from IncludePastEvents to IncludeState throughout tests for consistency

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
src/Worker/Grpc/GrpcInstanceRunnerUtils.cs New utility class containing shared logic for parsing request properties and initializing extended sessions cache
src/Worker/Grpc/GrpcEntityRunner.cs Added new overload with extended sessions support and implemented entity state caching logic
src/Worker/Grpc/GrpcOrchestrationRunner.cs Refactored to use shared utility for request property parsing and cache initialization
test/Worker/Grpc.Tests/GrpcEntityRunnerTests.cs New comprehensive test suite covering extended sessions scenarios for entities
test/Worker/Grpc.Tests/GrpcOrchestrationRunnerTests.cs Updated tests to use IncludeState parameter naming and improved variable naming consistency
Comments suppressed due to low confidence (1)

src/Worker/Grpc/GrpcOrchestrationRunner.cs:210

                    extendedSessions.Set<ExtendedSessionState>(

/// </summary>
/// <remarks>
/// If any request property is missing or invalid (i.e. the key is misspelled or the value is of the wrong type),
/// extended sessions are not enabled and default values are assigned are assigned to the returns.
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

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

Duplicate word "are assigned" in the documentation. Should be "extended sessions are not enabled and default values are assigned to the returns."

Suggested change
/// extended sessions are not enabled and default values are assigned are assigned to the returns.
/// extended sessions are not enabled and default values are assigned to the returns.

Copilot uses AI. Check for mistakes.

using System;
using System.Collections.Generic;
using System.Text;
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

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

Unused using statement for System.Text. This namespace appears to be included but not used anywhere in the file.

Suggested change
using System.Text;

Copilot uses AI. Check for mistakes.
byte[] requestBytes = entityRequest.ToByteArray();
string requestString = Convert.ToBase64String(requestBytes);
string responseString = await GrpcEntityRunner.LoadAndRunAsync(requestString, new SimpleEntity(), extendedSessions);
Protobuf.OrchestratorResponse response = Protobuf.OrchestratorResponse.Parser.ParseFrom(Convert.FromBase64String(responseString));
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

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

Incorrect response type used. The test parses the response as Protobuf.OrchestratorResponse when it should be Protobuf.EntityBatchResult (as used in the other tests).

Suggested change
Protobuf.OrchestratorResponse response = Protobuf.OrchestratorResponse.Parser.ParseFrom(Convert.FromBase64String(responseString));
Protobuf.EntityBatchResult response = Protobuf.EntityBatchResult.Parser.ParseFrom(Convert.FromBase64String(responseString));

Copilot uses AI. Check for mistakes.
addToExtendedSessions = true;

// If an entity state was provided, even if we already have one stored, we always want to use the provided state.
if (!entityStateIncluded && extendedSessions.TryGetValue(request.InstanceId, out string? entityState))
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

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

Missing null check for cached entity state. If a null value is stored in the cache (similar to the scenario tested in GrpcOrchestrationRunnerTests.NullExtendedSessionStored_Means_NeedsExtendedSessionNotUsed), this code will incorrectly set stateCached = true and use the null state. The orchestration runner has an explicit && extendedSessionState is not null check on line 149 of GrpcOrchestrationRunner.cs that should be mirrored here. The condition should be:

if (!entityStateIncluded && extendedSessions.TryGetValue(request.InstanceId, out string? entityState) && entityState is not null)
Suggested change
if (!entityStateIncluded && extendedSessions.TryGetValue(request.InstanceId, out string? entityState))
if (!entityStateIncluded && extendedSessions.TryGetValue(request.InstanceId, out string? entityState) && entityState is not null)

Copilot uses AI. Check for mistakes.
return entityBatchRequest;
}

class SimpleEntity : TaskEntity<int?>
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

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

Private test class should be sealed. According to the coding guidelines, "Ensure that all private classes, that do not serve as base classes, are sealed." This class should be declared as sealed class SimpleEntity : TaskEntity<int?>.

Copilot generated this review using guidance from repository custom instructions.

if (addToExtendedSessions)
{
extendedSessions.Set(
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

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

Variable extendedSessions may be null at this access as suggested by this null check.
Variable extendedSessions may be null at this access as suggested by this null check.

Suggested change
extendedSessions.Set(
extendedSessions?.Set(

Copilot uses AI. Check for mistakes.
byte[] requestBytes = entityRequest.ToByteArray();
string requestString = Convert.ToBase64String(requestBytes);
string responseString = await GrpcEntityRunner.LoadAndRunAsync(requestString, new SimpleEntity(), extendedSessions);
Protobuf.OrchestratorResponse response = Protobuf.OrchestratorResponse.Parser.ParseFrom(Convert.FromBase64String(responseString));
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

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

This assignment to response is useless, since its value is never read.

Suggested change
Protobuf.OrchestratorResponse response = Protobuf.OrchestratorResponse.Parser.ParseFrom(Convert.FromBase64String(responseString));

Copilot uses AI. Check for mistakes.
string stringResponse = GrpcOrchestrationRunner.LoadAndRun(requestString, new SimpleOrchestrator(), extendedSessions);
Protobuf.OrchestratorResponse response = Protobuf.OrchestratorResponse.Parser.ParseFrom(Convert.FromBase64String(stringResponse));
string responseString = GrpcOrchestrationRunner.LoadAndRun(requestString, new SimpleOrchestrator(), extendedSessions);
Protobuf.OrchestratorResponse response = Protobuf.OrchestratorResponse.Parser.ParseFrom(Convert.FromBase64String(responseString));
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

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

This assignment to response is useless, since its value is never read.

Suggested change
Protobuf.OrchestratorResponse response = Protobuf.OrchestratorResponse.Parser.ParseFrom(Convert.FromBase64String(responseString));

Copilot uses AI. Check for mistakes.
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