Review comments

Author: Sushisource

temporalio/lib/temporalio/internal/worker/workflow_instance.rb

@@ -24,6 +24,7 @@
 require 'temporalio/retry_policy'
 require 'temporalio/scoped_logger'
 require 'temporalio/worker/interceptor'
+require 'temporalio/worker_deployment_version'
 require 'temporalio/workflow/info'
 require 'temporalio/workflow/update_info'
 require 'timeout'
@@ -90,7 +91,7 @@ def initialize(details)
           @current_history_length = 0
           @current_history_size = 0
           @replaying = false
-          @failure_exception_types = details.workflow_failure_exception_types + @definition.failure_exception_types
+          @workflow_failure_exception_types = details.workflow_failure_exception_types
           @signal_handlers = HandlerHash.new(
             details.definition.signals,
             Workflow::Definition::Signal
@@ -324,14 +325,12 @@ def create_instance
                      end
 
           # Run Dynamic config getter
-          unless @definition.dynamic_options_method.nil?
-            dynamic_options = instance.send @definition.dynamic_options_method
-          end
-          unless dynamic_options.nil?
-            unless dynamic_options.versioning_behavior.nil?
+          if @definition.dynamic_options_method
+            dynamic_options = instance.send(@definition.dynamic_options_method)
+            if dynamic_options&.versioning_behavior
               @definition_options.versioning_behavior = dynamic_options.versioning_behavior
             end
-            unless dynamic_options.failure_exception_types.nil?
+            if dynamic_options&.failure_exception_types
               @definition_options.failure_exception_types = dynamic_options.failure_exception_types
             end
           end
@@ -650,9 +649,11 @@ def on_top_level_exception(err)
         end
 
         def failure_exception?(err)
-          err.is_a?(Error::Failure) || err.is_a?(Timeout::Error) || @failure_exception_types.any? do |cls|
-            err.is_a?(cls)
-          end
+          err.is_a?(Error::Failure) || err.is_a?(Timeout::Error) ||
+            ((@workflow_failure_exception_types || []) +
+             (@definition_options.failure_exception_types || [])).any? do |cls|
+              err.is_a?(cls)
+            end
         end
 
         def with_context_frozen(&)

temporalio/lib/temporalio/internal/worker/workflow_worker.rb

@@ -13,7 +13,7 @@ module Internal
     module Worker
       # Worker for handling workflow activations. Most activation work is delegated to the workflow executor.
       class WorkflowWorker
-        def self.workflow_definitions(workflows, should_enforce_versioning_behavior: false)
+        def self.workflow_definitions(workflows, should_enforce_versioning_behavior)
           workflows.each_with_object({}) do |workflow, hash|
             # Load definition
             defn = begin
@@ -31,7 +31,7 @@ def self.workflow_definitions(workflows, should_enforce_versioning_behavior: fal
 
             # Enforce versioning behavior is set when versioning is on
             if should_enforce_versioning_behavior &&
-               defn.versioning_behavior == VersioningBehavior::UNSPECIFIED && defn.dynamic_options_method.nil?
+               defn.versioning_behavior == VersioningBehavior::UNSPECIFIED && !defn.dynamic_options_method
               raise ArgumentError, "Workflow #{defn.name} must specify a versioning behavior"
             end
 

temporalio/lib/temporalio/worker.rb

@@ -33,7 +33,6 @@ class Worker
       :activity_executors,
       :workflow_executor,
       :interceptors,
-      :build_id,
       :identity,
       :logger,
       :max_cached_workflows,
@@ -47,7 +46,6 @@ class Worker
       :max_activities_per_second,
       :max_task_queue_activities_per_second,
       :graceful_shutdown_period,
-      :use_worker_versioning,
       :disable_eager_activity_execution,
       :illegal_workflow_calls,
       :workflow_failure_exception_types,
@@ -297,10 +295,6 @@ def self.default_illegal_workflow_calls
     # @param interceptors [Array<Interceptor::Activity, Interceptor::Workflow>] Interceptors specific to this worker.
     #   Note, interceptors set on the client that include the {Interceptor::Activity} or {Interceptor::Workflow} module
     #   are automatically included here, so no need to specify them again.
-    # @param build_id [String] Unique identifier for the current runtime. This is best set as a unique value
-    #   representing all code and should change only when code does. This can be something like a git commit hash. If
-    #   unset, default is hash of known Ruby code.
-    #   Exclusive with `deployment_options`.
     # @param identity [String, nil] Override the identity for this worker. If unset, client identity is used.
     # @param logger [Logger] Logger to override client logger with. Default is the client logger.
     # @param max_cached_workflows [Integer] Number of workflows held in cache for use by sticky task queue. If set to 0,
@@ -329,10 +323,6 @@ def self.default_illegal_workflow_calls
     #   multiple workers on the same queue have different values set, they will thrash with the last poller winning.
     # @param graceful_shutdown_period [Float] Amount of time after shutdown is called that activities are given to
     #   complete before their tasks are canceled.
-    # @param use_worker_versioning [Boolean] If true, the `build_id` argument must be specified, and this worker opts
-    #   into the worker versioning feature. This ensures it only receives workflow tasks for workflows which it claims
-    #   to be compatible with. For more information, see https://docs.temporal.io/workers#worker-versioning.
-    #   Exclusive with `deployment_options`.
     # @param disable_eager_activity_execution [Boolean] If true, disables eager activity execution. Eager activity
     #   execution is an optimization on some servers that sends activities back to the same worker as the calling
     #   workflow if they can run there. This should be set to true for `max_task_queue_activities_per_second` to work
@@ -355,7 +345,6 @@ def self.default_illegal_workflow_calls
     #   scheduler will fail. Instead of setting this to true, users are encouraged to use {Workflow::Unsafe.io_enabled}
     #   with a block for narrower enabling of IO.
     # @param deployment_options [DeploymentOptions, nil] Deployment options for the worker.
-    #   Exclusive with `build_id` and `use_worker_versioning`.
     #   WARNING: This is an experimental feature and may change in the future.
     # @param debug_mode [Boolean] If true, deadlock detection is disabled. Deadlock detection will fail workflow tasks
     #   if they block the thread for too long. This defaults to true if the `TEMPORAL_DEBUG` environment variable is
@@ -369,7 +358,6 @@ def initialize(
       activity_executors: ActivityExecutor.defaults,
       workflow_executor: WorkflowExecutor::ThreadPool.default,
       interceptors: [],
-      build_id: Worker.default_build_id,
       identity: nil,
       logger: client.options.logger,
       max_cached_workflows: 1000,
@@ -383,7 +371,6 @@ def initialize(
       max_activities_per_second: nil,
       max_task_queue_activities_per_second: nil,
       graceful_shutdown_period: 0,
-      use_worker_versioning: false,
       disable_eager_activity_execution: false,
       illegal_workflow_calls: Worker.default_illegal_workflow_calls,
       workflow_failure_exception_types: [],
@@ -405,7 +392,6 @@ def initialize(
         activity_executors:,
         workflow_executor:,
         interceptors:,
-        build_id:,
         identity:,
         logger:,
         max_cached_workflows:,
@@ -419,7 +405,6 @@ def initialize(
         max_activities_per_second:,
         max_task_queue_activities_per_second:,
         graceful_shutdown_period:,
-        use_worker_versioning:,
         disable_eager_activity_execution:,
         illegal_workflow_calls:,
         workflow_failure_exception_types:,
@@ -436,7 +421,7 @@ def initialize(
       # Preload workflow definitions and some workflow settings for the bridge
       workflow_definitions = Internal::Worker::WorkflowWorker.workflow_definitions(
         workflows,
-        should_enforce_versioning_behavior:
+        should_enforce_versioning_behavior
       )
       nondeterminism_as_workflow_fail, nondeterminism_as_workflow_fail_for_types =
         Internal::Worker::WorkflowWorker.bridge_workflow_failure_exception_type_options(
@@ -452,7 +437,7 @@ def initialize(
           namespace: client.namespace,
           task_queue:,
           tuner: tuner._to_bridge_options,
-          build_id:,
+          build_id: '',
           identity_override: identity,
           max_cached_workflows:,
           max_concurrent_workflow_task_polls:,
@@ -467,7 +452,7 @@ def initialize(
           max_worker_activities_per_second: max_activities_per_second,
           max_task_queue_activities_per_second:,
           graceful_shutdown_period:,
-          use_worker_versioning:,
+          use_worker_versioning: false,
           nondeterminism_as_workflow_fail:,
           nondeterminism_as_workflow_fail_for_types:,
           deployment_options: if deployment_options.nil?

temporalio/lib/temporalio/worker/deployment_options.rb

@@ -1,5 +1,8 @@
 # frozen_string_literal: true
 
+require 'temporalio/common_enums'
+require 'temporalio/worker_deployment_version'
+
 module Temporalio
   class Worker
     DeploymentOptions = Data.define(

temporalio/lib/temporalio/worker/workflow_replayer.rb

@@ -116,7 +116,7 @@ def initialize(
           runtime:
         ).freeze
         # Preload definitions and other settings
-        @workflow_definitions = Internal::Worker::WorkflowWorker.workflow_definitions(workflows)
+        @workflow_definitions = Internal::Worker::WorkflowWorker.workflow_definitions(workflows, false)
         @nondeterminism_as_workflow_fail, @nondeterminism_as_workflow_fail_for_types =
           Internal::Worker::WorkflowWorker.bridge_workflow_failure_exception_type_options(
             workflow_failure_exception_types:, workflow_definitions: @workflow_definitions

temporalio/lib/temporalio/worker_versioning.rb renamed to temporalio/lib/temporalio/worker_deployment_version.rb

@@ -1,5 +1,7 @@
 # frozen_string_literal: true
 
+require 'temporalio/internal/bridge/worker'
+
 module Temporalio
   WorkerDeploymentVersion = Data.define(
     :deployment_name,
@@ -12,6 +14,9 @@ module Temporalio
   class WorkerDeploymentVersion
     # Parse a version from a canonical string, which must be in the format
     # `<deployment_name>.<build_id>`. Deployment name must not have a `.` in it.
+    #
+    # @param canonical [String] The canonical string representation of the version.
+    # @return [WorkerDeploymentVersion] The parsed version.
     def self.from_canonical_string(canonical)
       parts = canonical.split('.', 2)
       if parts.length != 2
@@ -23,6 +28,8 @@ def self.from_canonical_string(canonical)
 
     # @!visibility private
     def self._from_bridge(bridge)
+      return nil if bridge.nil?
+
       new(deployment_name: bridge.deployment_name, build_id: bridge.build_id)
     end
 

temporalio/lib/temporalio/workflow/definition.rb

@@ -202,8 +202,6 @@ def workflow_update_validator(update_method)
         # will override those provided to other `workflow_xxx` setters.
         #
         # Cannot be specified on non-dynamic workflows.
-        #
-        # @param update_method [Symbol] Name of the dynamic options method.
         def workflow_dynamic_options
           raise 'Dynamic options method can only be set on workflows using `workflow_dynamic`' unless @workflow_dynamic
 
@@ -421,6 +419,10 @@ def self._build_workflow_definition
 
         raise 'Workflow cannot be given a name and be dynamic' if dynamic && override_name
 
+        if !dynamic && !@dynamic_options_method.nil?
+          raise 'Workflow cannot have a dynamic_options_method unless it is dynamic'
+        end
+
         Info.new(
           workflow_class: self,
           override_name:,

temporalio/test/test.rb

@@ -147,11 +147,7 @@ def initialize
         client = Temporalio::Client.connect(
           ENV['TEMPORAL_TEST_CLIENT_TARGET_HOST'],
           ENV['TEMPORAL_TEST_CLIENT_TARGET_NAMESPACE'] || 'default',
-          data_converter: Temporalio::Converters::DataConverter.default,
-          interceptors: [],
-          logger: Logger.new($stdout, level: Logger::WARN),
-          default_workflow_query_reject_condition: nil,
-          runtime: Temporalio::Runtime.default
+          logger: Logger.new($stdout)
         )
         @server = Temporalio::Testing::WorkflowEnvironment.new(client)
       else

temporalio/test/worker_activity_test.rb

@@ -660,7 +660,6 @@ def assert_multi_worker_activities(activities)
         client: env.client,
         task_queue: "tq-#{index}-#{SecureRandom.uuid}",
         activities: [activity],
-        build_id: 'ignore'
       )
     end
     Temporalio::Worker.run_all(*workers) do

temporalio/test/worker_workflow_versioning_test.rb

@@ -5,13 +5,13 @@
 require 'temporalio/testing'
 require 'temporalio/worker'
 require 'temporalio/worker/deployment_options'
-require 'temporalio/worker_versioning'
+require 'temporalio/worker_deployment_version'
 require 'temporalio/workflow'
 require 'temporalio/workflow/definition'
 require 'test'
 require 'timeout'
 
-class WorkerWorkflowTest < Test
+class WorkerWorkflowVersioningTest < Test
   class DeploymentVersioningWorkflowV1AutoUpgrade < Temporalio::Workflow::Definition
     workflow_name :DeploymentVersioningWorkflow
     workflow_versioning_behavior Temporalio::VersioningBehavior::AUTO_UPGRADE