ScheduleNewWorkflowAsync should accept `DateTimeOffset?` for `startTime`

[frankbuckley]

DurableTask StartOrchestrationOptions takes a DateTimeOffset? value for StartAt. However, DaprWorkflowClient.ScheduleNewWorkflowAsync accepts a DateTime?.

This is likely to lead to bugs where times are interpreted as being in the local time zone (where the workflow process is running) when the request to start the workflow may have originated from a user in a different time zone.

ScheduleNewWorkflowAsync should accept DateTimeOffset? for startTime.

Although this changes a shipped public method, it is arguably not breaking as it simply exposes the implicit conversion that is currently happening here (line 63):

dotnet-sdk/src/Dapr.Workflow/DaprWorkflowClient.cs

Lines 57 to 65 in e3efbad

	public Task<string> ScheduleNewWorkflowAsync(
	string name,
	string? instanceId = null,
	object? input = null,
	DateTime? startTime = null)
	{
	StartOrchestrationOptions options = new(instanceId, startTime);
	return this.innerClient.ScheduleNewOrchestrationInstanceAsync(name, input, options);
	}

[WhitWaldo]

This is actually sort of intentional. The date used internally by the workflow (and by the scheduler itself) is representative of the date on the server on which the runtime is executing and there is no translation being performed to modify between timezones at all (whether using actors, jobs or workflows). In other words, you might specify a DateTimeOffset reflecting a +6 UTC timezone, but the time that will be preserved will reflect the time reflected by the server itself.

It's because of this incongruity that we're explicitly not accepting DateTImeOffset so that this doesn't further confuse developers into expecting that an offset will be automatically calculated when it's not configured to do so.

That said, I'd be happy to take this as a feature request, but it's something that would need to be changed in the runtime (as it ultimately maintains the triggers itself and knows the local timezone used) and not something that can be supported in the SDK until it's in place there.

[frankbuckley]

Sorry if I am misunderstanding - I have only started using Workflow recently and I am still trying to understand how the sidecar scheduler works.

It seems that the DateTimeOffset is converted to UTC here:

        if (startAt.HasValue)
        {
            // Convert timestamps to UTC if not already UTC
            request.ScheduledStartTimestamp = Timestamp.FromDateTimeOffset(startAt.Value.ToUniversalTime());
        }


        P.CreateInstanceResponse? result = await this.sidecarClient.StartInstanceAsync(
            request, cancellationToken: cancellation);

https://github.com/dapr/durabletask-dotnet/blob/main/src/Client/Grpc/GrpcDurableTaskClient.cs#L135-L139

...so, if passed through as DateTimeOffset it would be interpreted correctly?

Or, does the runtime still interpret the Timestamp as local time?

[WhitWaldo]

The SDK is ultimately at the mercy of the runtime as it's just a wrapper for the orchestration happening there. I'll have to poke further into the code you cited, but here's the guidance (in Discord) from the runtime maintainer when I asked how the scheduler/jobs determined timezones and I don't believe this has changed:

We are using the clock.RealClock{} in the go-etcd-cron library, meaning jobs are scheduled based on the clock of the server where the scheduler is running. We are not converting to UTC. However, you can provide the timezone with the timestamp in the format of RFC3339, meaning you can define the timezone you'd like to adhere to. If you dontt provide the timezone then we use the server time

I don't know if Workflow's use of ScheduledStartTimestamp honors the RFC3339 format that scheduler supports, but I've pinged another runtime maintainer to validate. Should they confirm that the runtime would support a timezone presented in such a way, I've got no problem accepting a PR that allows that presentation to facilitate this.

[frankbuckley]

@WhitWaldo - thanks.

I cannot actually get workflow start time to work at all (dapr/dapr/issues/8855).

However, I can confirm I have debugged through to the Grpc request sent to the sidecar and the time is converted to UTC. Indeed, if it were not this would throw:

https://github.com/protocolbuffers/protobuf/blob/aee2170c92b95eefa2f6be8c915294cf3620eb1c/csharp/src/Google.Protobuf/WellKnownTypes/TimestampPartial.cs#L123-L133

If the server ends up treating the Timestamp as local time, that would (from what I can tell) be a bug. I cannot test it since every future start time I specify is ignored.

I think I would request adding documentation to the parameter, such as "If a DateTime value is specified with a Kind property of DateTimeKind.Unspecified, the DateTime is assumed to be local time" and then adding a more explicit overload (or corresponding extension method):

public Task<string> ScheduleNewWorkflowAsync(
    string name,
    string? instanceId = null,
    object? input = null,
    DateTimeOffset? startTime = null)
{
}

Changing the existing method would actually be breaking (contrary to what I initially suggested).

[WhitWaldo]

@frankbuckley I saw that Josh has put a PR together to fix this reported issue, so I expect that'll be in the next release.

I'll wait to hear from them on what this should mean with regards to the issue you originally raised here and then circle back with you on this.

[frankbuckley]

@WhitWaldo - now I can successfully test future start times, I can confirm runtime takes UTC Timestamp and interprets accordingly, so if you pass UTD or Local DateTime to ScheduleNewWorkflowAsync things work as expected. (If you pass DateTime unspecified, then it is interpreted as local when cast to DateTimeOffset when initialising the StartOrchestrationOptions.

Happy to submit PR for DateTimeOffset overload to make time zone handling easier to reason about if that is agreeable.

[WhitWaldo]

@WhitWaldo - now I can successfully test future start times, I can confirm runtime takes UTC Timestamp and interprets accordingly, so if you pass UTD or Local DateTime to ScheduleNewWorkflowAsync things work as expected. (If you pass DateTime unspecified, then it is interpreted as local when cast to DateTimeOffset when initialising the StartOrchestrationOptions.

Happy to submit PR for DateTimeOffset overload to make time zone handling easier to reason about if that is agreeable.

Sounds reasonable to me - I don't know if it'll get into the 1.16 release or not, but if not, I can fit it into a patch release shortly after.