Support certain Time calls in workflows with advanced validation
#291
Conversation
cretz
changed the title
Time calls in workflows with advanced validation
| # When the xmlschema (aliased as iso8601) call is made, zone_offset is called which has a default parameter | ||
| # of Time.now.year. We want to prevent failing in that specific case. It is expensive to access the caller | ||
| # stack, but this is only done in the rare case they are calling this safely. | ||
| next if caller_locations&.any? { |loc| loc.label == 'Time.zone_offset' } |
There was a problem hiding this comment.
Do we? I mean, it's technically possible that a WFT isn't picked up across a year boundary. Not even that hard to have it happen if it's happening right around NYE UTC time.
IE: WFT timestamp is just pre-midnight, then replay happens after midnight and we end up assuming it's that time except a year forward.
Or, is what's actually happening here that the year has to be provided and this call is still made anyway? Not immediately obvious to me.
There was a problem hiding this comment.
Do we?
IMO yes. Here's what's happening...
A user reported they can't even parse an ISO8601 time in a workflow (totally deterministic) because of this limitation. The line of code at https://github.com/ruby/ruby/blob/5124f9ac7513eb590c37717337c430cb93caa151/lib/time.rb#L640 is triggering it. During ISO-8601 parse the Ruby code needs zone offset, but obtaining zone offset (https://github.com/ruby/ruby/blob/5124f9ac7513eb590c37717337c430cb93caa151/lib/time.rb#L82) asks for current year as a parameter default it doesn't even really use for these cases.
There was a problem hiding this comment.
Ah, so, to clarify we're not actually getting the year from that, the call is just made anyway, even though we require the year to explicitly be set or set it ourselves?
There was a problem hiding this comment.
Exactly. It's a default parameter evaluated at invocation that is not even used in this case.
|
Just added a CI fix that added a line to basically every API file and one rubocop disabling to an unrelated bridge file. Can ignore those. |
2 participants
What was changed
Temporalio::Worker::IllegalWorkflowCallValidatorfor advanced call validation scenariosTime.newin workflows when a positional argument is presentTime.iso8601in workflowsChecklist