Address on-demand sandbox review comments

Make declaration helpers private, fail empty worker profiles, keep still-running registration thread handles, add worker typing, and add Bash examples to the sample README.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: YunchuWang

durabletask-azuremanaged/durabletask/azuremanaged/preview/on_demand_sandbox/client.py

@@ -7,7 +7,7 @@
 from azure.core.credentials import TokenCredential
 
 from durabletask.azuremanaged.preview.on_demand_sandbox.declarations import (
-    build_profile_on_demand_sandbox_activity_declarations,
+    _build_profile_on_demand_sandbox_activity_declarations,
 )
 from durabletask.azuremanaged.preview.on_demand_sandbox.transport import (
     OnDemandSandboxActivitiesGrpcTransport,
@@ -42,7 +42,7 @@ def close(self) -> None:
 
     def enable_on_demand_sandbox_activities(self) -> None:
         """Declare all configured on-demand sandbox worker profiles with Durable Task Scheduler."""
-        declarations = build_profile_on_demand_sandbox_activity_declarations()
+        declarations = _build_profile_on_demand_sandbox_activity_declarations()
         if not declarations:
             raise ValueError("No configured on-demand sandbox activities were found.")
 

durabletask-azuremanaged/durabletask/azuremanaged/preview/on_demand_sandbox/declarations.py

@@ -69,13 +69,17 @@ def decorator(cls: type) -> type:
         if callable(configure):
             configure(options)
 
+        if not _resolve_activity_names(options.activity_names):
+            raise ValueError(
+                f"On-demand sandbox worker profile '{normalized_profile}' must declare at least one activity.")
+
         _worker_profiles[normalized_profile] = options
         return cls
 
     return decorator
 
 
-def resolve_activity_names(activity_names: str | Iterable[str]) -> list[str]:
+def _resolve_activity_names(activity_names: str | Iterable[str]) -> list[str]:
     resolved: list[str] = []
     seen: set[str] = set()
     names = [activity_names] if isinstance(activity_names, str) else activity_names
@@ -87,7 +91,7 @@ def resolve_activity_names(activity_names: str | Iterable[str]) -> list[str]:
     return resolved
 
 
-def build_on_demand_sandbox_activity_declaration(
+def _build_on_demand_sandbox_activity_declaration(
         *,
         activity_names: str | Iterable[str],
         scheduler_managed_identity_client_id: str,
@@ -107,7 +111,7 @@ def build_on_demand_sandbox_activity_declaration(
             such as "myregistry.azurecr.io/workers/hello:1.0" or
             "myregistry.azurecr.io/workers/hello@sha256:0123456789abcdef...".
     """
-    resolved_activity_names = resolve_activity_names(activity_names)
+    resolved_activity_names = _resolve_activity_names(activity_names)
     if not resolved_activity_names:
         raise ValueError("On-demand sandbox activity declaration requires at least one activity name.")
 
@@ -150,24 +154,22 @@ def build_on_demand_sandbox_activity_declaration(
     return declaration
 
 
-def build_profile_on_demand_sandbox_activity_declarations() -> list[pb.OnDemandSandboxActivityDeclaration]:
+def _build_profile_on_demand_sandbox_activity_declarations() -> list[pb.OnDemandSandboxActivityDeclaration]:
     """Build on-demand sandbox declarations from worker profile configuration."""
     declarations: list[pb.OnDemandSandboxActivityDeclaration] = []
     activity_owners: dict[str, str] = {}
     for profile in _worker_profiles.values():
-        activity_names = resolve_activity_names(profile.activity_names)
-        if not activity_names:
-            continue
+        activity_names = _resolve_activity_names(profile.activity_names)
 
         for activity_name in activity_names:
             existing_profile = activity_owners.get(activity_name)
             if existing_profile and existing_profile != profile.worker_profile_id:
                 raise ValueError(
                     f"On-demand sandbox activity '{activity_name}' is assigned to both worker profile "
-                    f"'{existing_profile}' and '{profile.worker_profile_id}'.")
+                f"'{existing_profile}' and '{profile.worker_profile_id}'.")
             activity_owners[activity_name] = profile.worker_profile_id
 
-        declarations.append(build_on_demand_sandbox_activity_declaration(
+        declarations.append(_build_on_demand_sandbox_activity_declaration(
             activity_names=activity_names,
             worker_profile_id=profile.worker_profile_id,
             container_image=profile.container_image,
@@ -183,7 +185,7 @@ def build_profile_on_demand_sandbox_activity_declarations() -> list[pb.OnDemandS
     return declarations
 
 
-def build_on_demand_sandbox_worker_start(
+def _build_on_demand_sandbox_worker_start(
         *,
         taskhub: str,
         worker_profile_id: str,
@@ -200,7 +202,7 @@ def build_on_demand_sandbox_worker_start(
     if max_activities_count <= 0:
         raise ValueError("On-demand sandbox activity worker max activity count must be greater than zero.")
 
-    resolved_activity_names = resolve_activity_names(activity_names)
+    resolved_activity_names = _resolve_activity_names(activity_names)
     if not resolved_activity_names:
         raise ValueError("On-demand sandbox activity worker registration requires at least one registered activity.")
 
@@ -215,7 +217,7 @@ def build_on_demand_sandbox_worker_start(
     return message
 
 
-def build_on_demand_sandbox_worker_heartbeat(active_activities_count: int) -> pb.OnDemandSandboxActivityWorkerMessage:
+def _build_on_demand_sandbox_worker_heartbeat(active_activities_count: int) -> pb.OnDemandSandboxActivityWorkerMessage:
     if active_activities_count < 0:
         raise ValueError("On-demand sandbox activity worker active activity count cannot be negative.")
 

durabletask-azuremanaged/durabletask/azuremanaged/preview/on_demand_sandbox/worker.py

@@ -12,9 +12,9 @@
 from durabletask.azuremanaged.preview.on_demand_sandbox.declarations import (
     DEFAULT_MAX_CONCURRENT_ACTIVITIES,
     DEFAULT_WORKER_PROFILE_ID,
-    build_on_demand_sandbox_worker_heartbeat,
-    build_on_demand_sandbox_worker_start,
-    resolve_activity_names,
+    _build_on_demand_sandbox_worker_heartbeat,
+    _build_on_demand_sandbox_worker_start,
+    _resolve_activity_names,
 )
 from durabletask.azuremanaged.preview.on_demand_sandbox.transport import (
     OnDemandSandboxActivitiesGrpcTransport,
@@ -68,7 +68,7 @@ def __init__(self):
         self._on_demand_sandbox_active_activities = 0
         self._on_demand_sandbox_active_activities_lock = threading.Lock()
 
-    def add_activity(self, fn):
+    def add_activity(self, fn) -> str:
         activity_name = super().add_activity(fn)
         self._on_demand_sandbox_activity_names.append(activity_name)
         return activity_name
@@ -91,7 +91,7 @@ def _durabletask_on_activity_execution_completed(self, req) -> None:
             self._on_demand_sandbox_active_activities = max(0, self._on_demand_sandbox_active_activities - 1)
 
     def _configure_on_demand_sandbox_activity_filters(self) -> None:
-        activity_names = resolve_activity_names(self._on_demand_sandbox_activity_names)
+        activity_names = _resolve_activity_names(self._on_demand_sandbox_activity_names)
         if not activity_names:
             raise RuntimeError(
                 "On-demand sandbox worker requires at least one registered activity before it can register.")
@@ -112,8 +112,13 @@ def _start_on_demand_sandbox_registration(self) -> None:
 
     def _stop_on_demand_sandbox_registration(self) -> None:
         self._on_demand_sandbox_registration_stop.set()
-        if self._on_demand_sandbox_registration_thread is not None:
-            self._on_demand_sandbox_registration_thread.join(timeout=10)
+        thread = self._on_demand_sandbox_registration_thread
+        if thread is not None:
+            thread.join(timeout=10)
+            if thread.is_alive():
+                self._on_demand_sandbox_logger.warning(
+                    "On-demand sandbox activity worker registration thread did not stop within 10 seconds.")
+                return
             self._on_demand_sandbox_registration_thread = None
 
     def _run_on_demand_sandbox_registration_loop(self) -> None:
@@ -139,7 +144,7 @@ def _run_on_demand_sandbox_registration_loop(self) -> None:
                 retry_delay = min(retry_delay * 2, 30.0)
 
     def _registration_messages(self) -> Iterator:
-        yield build_on_demand_sandbox_worker_start(
+        yield _build_on_demand_sandbox_worker_start(
             taskhub=self._on_demand_sandbox_taskhub,
             worker_profile_id=self._on_demand_sandbox_worker_profile_id,
             max_activities_count=self._on_demand_sandbox_max_activities,
@@ -151,7 +156,7 @@ def _registration_messages(self) -> Iterator:
                 self._on_demand_sandbox_heartbeat_interval_seconds):
             with self._on_demand_sandbox_active_activities_lock:
                 active_count = self._on_demand_sandbox_active_activities
-            yield build_on_demand_sandbox_worker_heartbeat(active_count)
+            yield _build_on_demand_sandbox_worker_heartbeat(active_count)
 
 
 def _resolve_taskhub() -> str:

examples/on_demand_sandbox/README.md

@@ -30,6 +30,15 @@ $env:DTS_ON_DEMAND_SANDBOX_IMAGE_PULL_UMI_CLIENT_ID = "<image-pull UMI client ID
 $env:DTS_ON_DEMAND_SANDBOX_SCHEDULER_UMI_CLIENT_ID = "<scheduler UMI client ID>"
 ```
 
+```bash
+export DTS_ENDPOINT="<scheduler endpoint>"
+export DTS_TASK_HUB="<task hub name>"
+export DTS_WORKER_PROFILE_ID="default"
+export DTS_ON_DEMAND_SANDBOX_CONTAINER_IMAGE="<container image reference>"
+export DTS_ON_DEMAND_SANDBOX_IMAGE_PULL_UMI_CLIENT_ID="<image-pull UMI client ID>"
+export DTS_ON_DEMAND_SANDBOX_SCHEDULER_UMI_CLIENT_ID="<scheduler UMI client ID>"
+```
+
 After pushing the remote worker image, set `DTS_ON_DEMAND_SANDBOX_CONTAINER_IMAGE` to
 the pushed image reference. `RemoteWorkerProfile.configure()` declares CPU,
 memory, max concurrency, customer environment variables, and on-demand sandbox activity
@@ -55,6 +64,14 @@ docker build `
 docker push <public container image reference>
 ```
 
+```bash
+docker build \
+  -f examples/on_demand_sandbox/Containerfile \
+  -t <public container image reference> \
+  .
+docker push <public container image reference>
+```
+
 Private preview requires the image to be publicly pullable by the sandbox platform.
 
 ## Run the declarer app
@@ -65,12 +82,20 @@ Install local packages from the repository root:
 pip install -e . -e .\durabletask-azuremanaged
 ```
 
+```bash
+pip install -e . -e ./durabletask-azuremanaged
+```
+
 Then run:
 
 ```powershell
 python examples\on_demand_sandbox\main_app.py
 ```
 
+```bash
+python examples/on_demand_sandbox/main_app.py
+```
+
 The declarer app registers the on-demand sandbox activity metadata, starts
 `hello_orchestrator`, and the remote worker sandbox executes `remote_hello`.
 The result includes `ON_DEMAND_SANDBOX_SAMPLE_MARKER=on-demand-sandbox-python-sample-marker`,