Skip to content

Conversation

@bobhauser
Copy link
Contributor

@bobhauser bobhauser commented Oct 17, 2025

This PR fixes an issue where workflows could not persist internal state globally when the workflow default persistence mode was set to Exclude. The internalState setting defined at the workflow level was ignored, causing internal state to inherit the workflow default for inputs and outputs rather than its own configuration.

Issue: #6979


This change is Reviewable

@bobhauser bobhauser changed the base branch from main to patch/3.5.2 October 17, 2025 19:09
@sfmskywalker
Copy link
Member

sfmskywalker commented Nov 11, 2025

@bobhauser The diff is so big that I can't really get a good look at the actual changes. Is there any chance you can push only the relevant change without the reformatting?

@sfmskywalker sfmskywalker added this to the 3.5.2 milestone Nov 11, 2025
@sfmskywalker sfmskywalker requested a review from Copilot November 12, 2025 07:52
Copilot finished reviewing on behalf of sfmskywalker November 12, 2025 07:54
Copy link
Contributor

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 fixes an issue where internal state persistence configuration was incorrectly inheriting from the workflow's default persistence mode for inputs/outputs, rather than having its own independent configuration hierarchy. The fix ensures that when the workflow default persistence mode is set to "Exclude", internal state can still be configured to persist globally using its own internalState configuration key.

Key Changes:

  • Refactored persistence mode resolution to handle internal state separately from inputs/outputs
  • Replaced the specialized EvaluateInternalStateModeAsync method with a generalized GetPersistenceModeAsync method
  • Introduced new constants for configuration keys to improve maintainability

Comment on lines +103 to +106
// Resolve internalStateMode for internal state. Resolution hierarchy: activity internal state -> activity default -> workflow internal state -> workflow default -> system default from options.
var workflowInternalStateDefaultMode = await GetPersistenceModeAsync(rootContext.ExpressionExecutionContext, workflow.CustomProperties, InternalStatePersistenceKey, () => workflowDefaultMode, cancellationToken);
var activityDefaultMode = await GetPersistenceModeAsync(context.ExpressionExecutionContext, context.Activity.CustomProperties, DefaultPersistenceKey, () => workflowInternalStateDefaultMode, cancellationToken);
var internalStateMode = await GetPersistenceModeAsync(context.ExpressionExecutionContext, context.Activity.CustomProperties, InternalStatePersistenceKey, () => activityDefaultMode, cancellationToken);
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

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

The resolution hierarchy comment on line 103 states the order as 'activity internal state -> activity default -> workflow internal state -> workflow default -> system default', but the implementation on lines 104-106 creates a different chain. Line 105 retrieves the activity's 'default' key with workflowInternalStateDefaultMode as fallback, meaning if the activity has a 'default' configured, it will override the workflow's 'internalState' setting. The actual hierarchy implemented is: activity internalState → activity default → workflow internalState → workflow default → system default. However, the comment suggests: activity internalState → workflow internalState → workflow default → activity default → system default. Consider either updating the comment to accurately reflect the implementation or adjusting the implementation to match the intended hierarchy.

Suggested change
// Resolve internalStateMode for internal state. Resolution hierarchy: activity internal state -> activity default -> workflow internal state -> workflow default -> system default from options.
var workflowInternalStateDefaultMode = await GetPersistenceModeAsync(rootContext.ExpressionExecutionContext, workflow.CustomProperties, InternalStatePersistenceKey, () => workflowDefaultMode, cancellationToken);
var activityDefaultMode = await GetPersistenceModeAsync(context.ExpressionExecutionContext, context.Activity.CustomProperties, DefaultPersistenceKey, () => workflowInternalStateDefaultMode, cancellationToken);
var internalStateMode = await GetPersistenceModeAsync(context.ExpressionExecutionContext, context.Activity.CustomProperties, InternalStatePersistenceKey, () => activityDefaultMode, cancellationToken);
// Resolve internalStateMode for internal state. Resolution hierarchy: activity internal state -> workflow internal state -> workflow default -> activity default -> system default from options.
var workflowInternalStateDefaultMode = await GetPersistenceModeAsync(rootContext.ExpressionExecutionContext, workflow.CustomProperties, InternalStatePersistenceKey, () => workflowDefaultMode, cancellationToken);
var activityDefaultMode = await GetPersistenceModeAsync(context.ExpressionExecutionContext, context.Activity.CustomProperties, DefaultPersistenceKey, () => workflowDefaultMode, cancellationToken);
var internalStateMode = await GetPersistenceModeAsync(
context.ExpressionExecutionContext,
context.Activity.CustomProperties,
InternalStatePersistenceKey,
() => workflowInternalStateDefaultMode,
cancellationToken
);

Copilot uses AI. Check for mistakes.
Comment on lines +222 to +226
catch (Exception ex)
{
_logger.LogWarning(ex, "Error evaluating log persistence expression");
return defaultFactory();
}
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

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

Generic catch clause.

Suggested change
catch (Exception ex)
{
_logger.LogWarning(ex, "Error evaluating log persistence expression");
return defaultFactory();
}
catch (InvalidOperationException ex)
{
_logger.LogWarning(ex, "Error evaluating log persistence expression");
return defaultFactory();
}
catch (ArgumentException ex)
{
_logger.LogWarning(ex, "Error evaluating log persistence expression");
return defaultFactory();
}

Copilot uses AI. Check for mistakes.
@sfmskywalker
Copy link
Member

I'm moving this out of scope for 3.5.2 for now, as I'm not clear about the issue. The diff contains too many changes for me to grok due to (I think) formatting changes.

@sfmskywalker sfmskywalker removed this from the 3.5.2 milestone Nov 18, 2025
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.

3 participants