Ondemand sandbox#736
Conversation
ServerlessActivitiesClientAdapter now takes an attachTaskHubMetadata flag (default true). The Azure-managed channel already injects the taskhub header via CallCredentials, so the AddDurableTaskScheduler* path constructs the adapter with attachTaskHubMetadata: false to avoid sending duplicate headers on DeclareServerlessActivities and ConnectServerlessActivityWorker. Added two unit tests with a recording CallInvoker covering both modes.
This reverts commit 18c8e02.
- Updated README.md to clarify remote worker image settings. - Simplified task hub retrieval in Program.cs. - Removed unnecessary endpoint configuration in remote worker. - Added Azure.Identity package reference in csproj. - Refined serverless worker extensions for environment configuration. - Updated serverless activity configuration to handle registered activities. - Modified tests to reflect changes in activity registration and filtering.
There was a problem hiding this comment.
⚠️ Not ready to approve
Resource validation currently truncates fractional memory quantities during MiB conversion, which can allow invalid configurations to pass SDK-side checks.
Copilot's findings
- Files reviewed: 53/53 changed files
- Comments generated: 1
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
There was a problem hiding this comment.
⚠️ Not ready to approve
A confirmed shutdown/registration race exists where the sandbox worker registration session can be disposed without synchronizing with in-flight stream completion, risking non-graceful teardown.
Copilot's findings
- Files reviewed: 53/53 changed files
- Comments generated: 1
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
There was a problem hiding this comment.
⚠️ Human review recommended
It introduces new gRPC contracts, new NuGet packages, worker execution hooks, and a multi-component sample/publish pipeline update that warrants final human review.
Copilot's findings
- Files reviewed: 53/53 changed files
- Comments generated: 2
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
There was a problem hiding this comment.
⚠️ Not ready to approve
The gRPC worker’s new activity notification calls reference this.NotifyActivity(...) even though NotifyActivity is introduced as a local function, which will not compile.
Copilot's findings
- Files reviewed: 53/53 changed files
- Comments generated: 2
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
halspang
left a comment
halspang
left a comment
There was a problem hiding this comment.
Just some nits, otherwise looks good!
There was a problem hiding this comment.
⚠️ Not ready to approve
GrpcDurableTaskWorker.Processor.cs currently calls a local function via this.NotifyActivity(...), which will not compile and must be corrected before merge.
Copilot's findings
- Files reviewed: 53/53 changed files
- Comments generated: 2
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
The scenario
A .NET app using Durable Task Scheduler should be able to keep its normal orchestration worker local, but run selected expensive or isolated activities in a DTS-started sandbox worker container.
The intended flow is:
UseSandboxWorker(), connects back to DTS, reports the activities it registered, and heartbeats active activity count.In the sample, the local app runs the orchestration and
LocalHello; the sandbox worker image runsRemoteHello. Both processes connect to the same task hub, but they must not receive the same work.What's missing
durabletask-dotnetdid not have a .NET SDK surface for this pattern.There was no client package for declaring sandbox worker profiles, no worker package for code running inside the sandbox, and no shared .NET transport for the sandbox activity protobuf service.
A few correctness details also fell out during validation:
UseWorkItemFilters(). Without it, DTS can dispatchRemoteHelloto the local worker, which does not implement that activity, so the orchestration can get stuck retrying the wrong worker.DTS_SANDBOX_IDmakes server-side routing and telemetry hard to reason about.The change
The SDK surface is split into two opt-in preview packages:
Microsoft.DurableTask.Client.AzureManaged.SandboxesMicrosoft.DurableTask.Worker.AzureManaged.SandboxesA good review path is:
src/Grpc/sandbox_service.protoandsrc/Shared/AzureManaged.Sandboxes/. This is the shared contract and transport for declaring sandbox activities, removing declarations, and connecting a live sandbox worker stream.src/Client/AzureManaged.Sandboxes/. This is the local declarer-app side:[SandboxWorkerProfile],SandboxWorkerProfileOptions, declaration discovery, validation, andSandboxActivitiesClient.src/Worker/AzureManaged.Sandboxes/. This is the sandbox-worker side:UseSandboxWorker(), activity-only filters, DTS-injected runtime settings, start/heartbeat messages, and active activity tracking.src/Worker/Grpc/. These add guarded activity execution notifications used by the sandbox worker heartbeat path.samples/on-demand-sandbox/last. It shows the intended end-to-end shape: local declarer app, remote worker image, and shared activity names.The new sandbox packages target
net10.0only because this is new preview functionality. NuGet publish jobs are split per package so an already-published AzureManaged package version does not block publishing the new Sandboxes packages.