Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions temporalio/.rubocop.yml
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ Metrics/BlockNesting:
# The default is too small
Metrics/ClassLength:
Max: 1000
Exclude:
- test/**/*

# The default is too small
Metrics/CyclomaticComplexity:
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
# frozen_string_literal: true

require 'temporalio/worker/illegal_workflow_call_validator'
require 'temporalio/workflow'

module Temporalio
Expand All @@ -12,16 +13,32 @@ def self.frozen_validated_illegal_calls(illegal_calls)
illegal_calls.to_h do |key, val|
raise TypeError, 'Invalid illegal call map, top-level key must be a String' unless key.is_a?(String)

# @type var fixed_val: :all | Hash[Symbol, bool]
# @type var fixed_val: :all | Worker::IllegalWorkflowCallValidator | Hash[Symbol, TrueClass | Worker::IllegalWorkflowCallValidator] # rubocop:disable Layout/LineLength
fixed_val = case val
when Temporalio::Worker::IllegalWorkflowCallValidator
if sub_val.method_name
raise ArgumentError,
'Top level IllegalWorkflowCallValidator instances cannot have method name'
end
val
when Array
val.to_h do |sub_val|
unless sub_val.is_a?(Symbol)
case sub_val
when Symbol
[sub_val, true]
when Temporalio::Worker::IllegalWorkflowCallValidator
unless sub_val.method_name
raise ArgumentError,
'IllegalWorkflowCallValidator instances in array for ' \
"#{key} must have a method name"
end

[sub_val.method_name, sub_val]
else
raise TypeError,
'Invalid illegal call map, each value must be a Symbol'
'Invalid illegal call array entry for ' \
"#{key}, each value must be a Symbol or an IllegalWorkflowCallValidator"
end

[sub_val, true]
end.freeze
when :all
:all
Expand All @@ -47,25 +64,54 @@ def initialize(illegal_calls)
# class of things like `Date` does not have `attached_object` so you have to fall back in these rare cases
# to parsing the string output. Reaching the string parsing component is rare, so this should not have
# significant performance impact.
cls_name = if cls.singleton_class?
if cls.respond_to?(:attached_object)
cls = cls.attached_object # steep:ignore
next unless cls.is_a?(Module)
class_name = if cls.singleton_class?
if cls.respond_to?(:attached_object)
cls = cls.attached_object # steep:ignore
next unless cls.is_a?(Module)

cls.name.to_s
cls.name.to_s
else
cls.to_s.delete_prefix('#<Class:').delete_suffix('>')
end
else
cls.to_s.delete_prefix('#<Class:').delete_suffix('>')
cls.name.to_s
end
else
cls.name.to_s
end

# Check if the call is considered illegal
vals = illegal_calls[cls_name]
if vals == :all || vals&.[](tp.callee_id) # steep:ignore
vals = illegal_calls[class_name]
invalid_suffix =
case vals
when :all
''
when Temporalio::Worker::IllegalWorkflowCallValidator
disable do
vals.block.call(Temporalio::Worker::IllegalWorkflowCallValidator::CallInfo.new(
class_name:, method_name: tp.callee_id, trace_point: tp
))
nil
rescue Exception => e # rubocop:disable Lint/RescueException
", reason: #{e}"
end
else
per_method = vals&.[](tp.callee_id)
case per_method
when true
''
when Temporalio::Worker::IllegalWorkflowCallValidator
disable do
per_method.block.call(Temporalio::Worker::IllegalWorkflowCallValidator::CallInfo.new(
class_name:, method_name: tp.callee_id, trace_point: tp
))
nil
rescue Exception => e # rubocop:disable Lint/RescueException
", reason: #{e}"
end
end
end
if invalid_suffix
raise Workflow::NondeterminismError,
"Cannot access #{cls_name} #{tp.callee_id} from inside a " \
'workflow. If this is known to be safe, the code can be run in ' \
"Cannot access #{class_name} #{tp.callee_id} from inside a " \
"workflow#{invalid_suffix}. If this is known to be safe, the code can be run in " \
'a Temporalio::Workflow::Unsafe.illegal_call_tracing_disabled block.'
end
end
Expand Down
21 changes: 13 additions & 8 deletions temporalio/lib/temporalio/worker.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
require 'temporalio/internal/worker/workflow_instance'
require 'temporalio/internal/worker/workflow_worker'
require 'temporalio/worker/activity_executor'
require 'temporalio/worker/illegal_workflow_call_validator'
require 'temporalio/worker/interceptor'
require 'temporalio/worker/poller_behavior'
require 'temporalio/worker/thread_pool'
Expand Down Expand Up @@ -242,8 +243,9 @@ def self.run_all(
end
end

# @return [Hash<String, [:all, Array<Symbol>]>] Default, immutable set illegal calls used for the
# `illegal_workflow_calls` worker option. See the documentation of that option for more details.
# @return [Hash<String, [:all, Array<Symbol, IllegalWorkflowCallValidator>, IllegalWorkflowCallValidator]>] Default,
# immutable set illegal calls used for the `illegal_workflow_calls` worker option. See the documentation of that
# option for more details.
def self.default_illegal_workflow_calls
@default_illegal_workflow_calls ||= begin
hash = {
Expand Down Expand Up @@ -282,7 +284,7 @@ def self.default_illegal_workflow_calls
'Thread' => %i[abort_on_exception= exit fork handle_interrupt ignore_deadlock= kill new pass
pending_interrupt? report_on_exception= start stop initialize join name= priority= raise run
terminate thread_variable_set wakeup],
'Time' => %i[initialize now]
'Time' => IllegalWorkflowCallValidator.default_time_validators
} #: Hash[String, :all | Array[Symbol]]
hash.each_value(&:freeze)
hash.freeze
Expand Down Expand Up @@ -339,11 +341,14 @@ def self.default_illegal_workflow_calls
# workflow if they can run there. This should be set to true for `max_task_queue_activities_per_second` to work
# and in a future version of this API may be implied as such (i.e. this setting will be ignored if that setting is
# set).
# @param illegal_workflow_calls [Hash<String, [:all, Array<Symbol>]>] Set of illegal workflow calls that are
# considered unsafe/non-deterministic and will raise if seen. The key of the hash is the fully qualified string
# class name (no leading `::`). The value is either `:all` which means any use of the class, or an array of
# symbols for methods on the class that cannot be used. The methods refer to either instance or class methods,
# there is no way to differentiate at this time.
# @param illegal_workflow_calls [Hash<String,
# [:all, Array<Symbol, IllegalWorkflowCallValidator>, IllegalWorkflowCallValidator]>] Set of illegal workflow
# calls that are considered unsafe/non-deterministic and will raise if seen. The key of the hash is the fully
# qualified string class name (no leading `::`). The value can be `:all` which means any use of the class is
# illegal. The value can be an array of symbols/validators for methods on the class that cannot be used. The
# methods refer to either instance or class methods, there is no way to differentiate at this time. Symbol method
# names are the normal way to say the method cannot be used, validators are only for advanced situations. Finally,
# for advanced situations, the hash value can be a class-level validator that is not tied to a specific method.
# @param workflow_failure_exception_types [Array<Class<Exception>>] Workflow failure exception types. This is the
# set of exception types that, if a workflow-thrown exception extends, will cause the workflow/update to fail
# instead of suspending the workflow via task failure. These are applied in addition to the
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
# frozen_string_literal: true

module Temporalio
class Worker
# Custom validator for validating illegal workflow calls.
class IllegalWorkflowCallValidator
CallInfo = Data.define(
:class_name,
:method_name,
:trace_point
)

# Call info passed to the validation block for each validation.
#
# @!attribute class_name
# @return [String] Class name the method is on.
# @!attribute method_name
# @return [String] Method name being called.
# @!attribute trace_point
# @return [TracePoint] TracePoint instance for the call.
class CallInfo; end # rubocop:disable Lint/EmptyClass

# @return [Array<IllegalWorkflowCallValidator>] Set of advanced validators for Time calls.
def self.default_time_validators
@default_time_validators ||= [
# Do not consider initialize as invalid if year is present and not "true"
IllegalWorkflowCallValidator.new(method_name: :initialize) do |info|
year_val = info.trace_point.binding&.local_variable_get(:year)
raise 'can only use if passing string or explicit time info' unless year_val && year_val != true
end,
IllegalWorkflowCallValidator.new(method_name: :now) do
# 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' }
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@cretz cretz Jun 27, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly. It's a default parameter evaluated at invocation that is not even used in this case.


raise 'Invalid Time.now call'
end
]
end

# @return [String, nil] Method name if this validator is specific to a method.
attr_reader :method_name

# @return [Proc] Block provided in constructor to invoke. See constructor for more details.
attr_reader :block

# Create a call validator.
#
# @param method_name [String, nil] Method name to check. This must be provided if the validator is in an illegal
# call array, this cannot be provided if it is a top-level class validator.
# @yield Required block that is called each time validation is needed. If the call raises, the exception message
# is used as the reason why the call is considered invalid. Return value is ignored.
# @yieldparam info [CallInfo] Information about the current call.
def initialize(method_name: nil, &block)
raise 'Block required' unless block_given?
raise TypeError, 'Method name must be Symbol' unless method_name.nil? || method_name.is_a?(Symbol)

@method_name = method_name
@block = block
end
end
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ module Temporalio
attr_reader failure_converter: Converters::FailureConverter
attr_reader interceptors: Array[Temporalio::Worker::Interceptor::Workflow]
attr_reader disable_eager_activity_execution: bool
attr_reader illegal_calls: Hash[String, :all | Hash[Symbol, bool]]
attr_reader illegal_calls: Hash[String, :all | Hash[Symbol, TrueClass | Temporalio::Worker::IllegalWorkflowCallValidator] | Temporalio::Worker::IllegalWorkflowCallValidator]
attr_reader workflow_failure_exception_types: Array[singleton(Exception)]
attr_reader unsafe_workflow_io_enabled: bool
attr_reader assert_valid_local_activity: ^(String) -> void
Expand All @@ -29,7 +29,7 @@ module Temporalio
failure_converter: Converters::FailureConverter,
interceptors: Array[Temporalio::Worker::Interceptor::Workflow],
disable_eager_activity_execution: bool,
illegal_calls: Hash[String, :all | Hash[Symbol, bool]],
illegal_calls: Hash[String, :all | Hash[Symbol, TrueClass | Temporalio::Worker::IllegalWorkflowCallValidator] | Temporalio::Worker::IllegalWorkflowCallValidator],
workflow_failure_exception_types: Array[singleton(Exception)],
unsafe_workflow_io_enabled: bool,
assert_valid_local_activity: ^(String) -> void
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,12 @@ module Temporalio
class WorkflowInstance
class IllegalCallTracer
def self.frozen_validated_illegal_calls: (
Hash[String, :all | Array[Symbol]] illegal_calls
) -> Hash[String, :all | Hash[Symbol, bool]]
Hash[String, :all | Array[Symbol | Temporalio::Worker::IllegalWorkflowCallValidator] | Temporalio::Worker::IllegalWorkflowCallValidator] illegal_calls
) -> Hash[String, :all | Hash[Symbol, TrueClass | Temporalio::Worker::IllegalWorkflowCallValidator] | Temporalio::Worker::IllegalWorkflowCallValidator]

def initialize: (Hash[String, :all | Hash[Symbol, bool]] illegal_calls) -> void
def initialize: (
Hash[String, :all | Hash[Symbol, TrueClass | Temporalio::Worker::IllegalWorkflowCallValidator] | Temporalio::Worker::IllegalWorkflowCallValidator] illegal_calls
) -> void

def enable: [T] { -> T } -> T
def disable: [T] { -> T } -> T
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ module Temporalio
metric_meter: Temporalio::Metric::Meter,
workflow_interceptors: Array[Temporalio::Worker::Interceptor::Workflow],
disable_eager_activity_execution: bool,
illegal_workflow_calls: Hash[String, :all | Array[Symbol]],
illegal_workflow_calls: Hash[String, :all | Array[Symbol | Temporalio::Worker::IllegalWorkflowCallValidator] | Temporalio::Worker::IllegalWorkflowCallValidator],
workflow_failure_exception_types: Array[singleton(Exception)],
workflow_payload_codec_thread_pool: Temporalio::Worker::ThreadPool?,
unsafe_workflow_io_enabled: bool,
Expand Down Expand Up @@ -58,7 +58,7 @@ module Temporalio
attr_reader metric_meter: Temporalio::Metric::Meter
attr_reader data_converter: Converters::DataConverter
attr_reader deadlock_timeout: Float?
attr_reader illegal_calls: Hash[String, :all | Hash[Symbol, bool]]
attr_reader illegal_calls: Hash[String, :all | Hash[Symbol, TrueClass | Temporalio::Worker::IllegalWorkflowCallValidator] | Temporalio::Worker::IllegalWorkflowCallValidator]
attr_reader namespace: String
attr_reader task_queue: String
attr_reader disable_eager_activity_execution: bool
Expand All @@ -76,7 +76,7 @@ module Temporalio
metric_meter: Temporalio::Metric::Meter,
data_converter: Converters::DataConverter,
deadlock_timeout: Float?,
illegal_calls: Hash[String, :all | Hash[Symbol, bool]],
illegal_calls: Hash[String, :all | Hash[Symbol, TrueClass | Temporalio::Worker::IllegalWorkflowCallValidator] | Temporalio::Worker::IllegalWorkflowCallValidator],
namespace: String,
task_queue: String,
disable_eager_activity_execution: bool,
Expand Down
8 changes: 4 additions & 4 deletions temporalio/sig/temporalio/worker.rbs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ module Temporalio
attr_reader max_task_queue_activities_per_second: Float?
attr_reader graceful_shutdown_period: Float
attr_reader disable_eager_activity_execution: bool
attr_reader illegal_workflow_calls: Hash[String, :all | Array[Symbol]]
attr_reader illegal_workflow_calls: Hash[String, :all | Array[Symbol | IllegalWorkflowCallValidator] | IllegalWorkflowCallValidator]
attr_reader workflow_failure_exception_types: Array[singleton(Exception)]
attr_reader workflow_payload_codec_thread_pool: ThreadPool?
attr_reader unsafe_workflow_io_enabled: bool
Expand Down Expand Up @@ -55,7 +55,7 @@ module Temporalio
max_task_queue_activities_per_second: Float?,
graceful_shutdown_period: Float,
disable_eager_activity_execution: bool,
illegal_workflow_calls: Hash[String, :all | Array[Symbol]],
illegal_workflow_calls: Hash[String, :all | Array[Symbol | IllegalWorkflowCallValidator] | IllegalWorkflowCallValidator],
workflow_failure_exception_types: Array[singleton(Exception)],
workflow_payload_codec_thread_pool: ThreadPool?,
unsafe_workflow_io_enabled: bool,
Expand All @@ -80,7 +80,7 @@ module Temporalio
?wait_block_complete: bool
) ?{ -> T } -> T

def self.default_illegal_workflow_calls: -> Hash[String, :all | Array[Symbol]]
def self.default_illegal_workflow_calls: -> Hash[String, :all | Array[Symbol | IllegalWorkflowCallValidator] | IllegalWorkflowCallValidator]

attr_reader options: Options

Expand All @@ -107,7 +107,7 @@ module Temporalio
?max_task_queue_activities_per_second: Float?,
?graceful_shutdown_period: Float,
?disable_eager_activity_execution: bool,
?illegal_workflow_calls: Hash[String, :all | Array[Symbol]],
?illegal_workflow_calls: Hash[String, :all | Array[Symbol | IllegalWorkflowCallValidator] | IllegalWorkflowCallValidator],
?workflow_failure_exception_types: Array[singleton(Exception)],
?workflow_payload_codec_thread_pool: ThreadPool?,
?unsafe_workflow_io_enabled: bool,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
module Temporalio
class Worker
class IllegalWorkflowCallValidator
class CallInfo
attr_reader class_name: String
attr_reader method_name: Symbol
attr_reader trace_point: TracePoint

def initialize: (
class_name: String,
method_name: Symbol,
trace_point: TracePoint
) -> void
end

def self.default_time_validators: -> Array[IllegalWorkflowCallValidator]

attr_reader method_name: Symbol?
attr_reader block: ^(CallInfo) -> void

def initialize: (?method_name: Symbol?) { (CallInfo) -> void } -> void
end
end
end
Loading
Loading