Implement sandbox exec through command router by ehdr · Pull Request #238 · modal-labs/libmodal

ehdr · 2025-12-16T17:23:56Z

Port modal-labs/modal-client@6133e091 / modal-labs/modal-client#3673 to add direct worker connections for Sandbox
exec operations, reducing latency and improving reliability.

The first commit bumps the modal-client submodule and refreshes protos, sorry for the noisy commit. Probably best to exclude that commit from the diff when revieweing.

Note

Switches Sandbox exec to the task command router with direct worker connections, adds client/config support, updates tests, and refreshes protos with related fields/messages.

JS Client/Sandbox:
- Implement exec via TaskCommandRouterClientImpl (direct worker connections), replacing container-exec path.
- Add validateExecArgs, deadline handling, PTY mapping, and binary/text I/O streaming via router.
- New buildTaskExecStartRequestProto; update ContainerProcess I/O to use router.
- Config: support task_command_router_insecure (env: MODAL_TASK_COMMAND_ROUTER_INSECURE).
- Expose timeoutMiddleware; minor CloudBucketMount.toProto create() usage.
Protos (refresh):
- Add HTTPConfig and fields to Function/FunctionData.
- Enhance CloudBucketMount (force_path_style, metadata TTL oneof/enum).
- Add AppCreateRequest.tags, AppGetLogsRequest.parametrized_function_id, CancelInputEvent.cancellation_reason.
- Environment concurrency fields; Proxy region; add RPCRetryPolicy/RPCStatus.
- Remove Sandbox router protos; extend Task command router (mount/snapshot dir).
Tests:
- Update Sandbox exec tests to router path; add task command router client tests.
Changelog:
- Note internal switch of Sandbox exec to new command router for performance/reliability.

^{Written by Cursor Bugbot for commit 4580ac9. This will update automatically on new commits. Configure here.}

Port modal-labs/modal-client@6133e091 to add direct worker connections for Sandbox exec operations, reducing latency and improving reliability.

saltzm

Looks pretty good overall! Main thing is I think it'd be good to add some more tests

saltzm · 2025-12-16T19:49:57Z

modal-js/src/sandbox.ts

+    stdoutConfig = TaskExecStdoutConfig.TASK_EXEC_STDOUT_CONFIG_DEVNULL;
+  } else {
+    throw new Error(`Unsupported stdout behavior: ${stdout}`);
+  }


Does the JS API not support "stdout" as an option?

in SandboxExecParams you can pass stdout, and it can be one of StdioBehavior = "pipe" | "ignore", does that make sense?

Ah - I meant stdout = "stdout" 😄 There's a third option in the python client: https://github.com/modal-labs/modal-client/blob/efe8b48bbd66ba204447de2f035614870dfb1a6a/modal/stream_type.py#L12

Not adamant we support it here, I don't think it's commonly used, though a couple customers do use it. It just prints everything from the exec stdout to local stdout

saltzm · 2025-12-16T19:51:08Z

modal-js/src/sandbox.ts

    command: string[],
    params?: SandboxExecParams & { mode?: "text" },
-  ): Promise<ContainerProcess<string>>;
+  ): Promise<ContainerProcess<string> | ContainerProcessThroughRouter<string>>;


Is this a breaking API change? If so does it matter? If we're okay doing this, we could maybe just remove the old ContainerProcess object, since I don't think we're going back to the old exec implementation at this point. Fine doing that in stages though if we want to play it safe

Ah good, I'll remove the old implementation then!

modal-js/src/sandbox.ts

saltzm · 2025-12-16T19:56:19Z

modal-js/src/sandbox.ts

+      ),
+    );
+    if (stdout === "ignore") {
+      stdoutStream.cancel();


Could we just avoid constructing the above stdoutStream instead?

saltzm · 2025-12-16T19:56:25Z

modal-js/src/sandbox.ts

+      ),
+    );
+    if (stderr === "ignore") {
+      stderrStream.cancel();


saltzm · 2025-12-16T20:00:13Z

modal-js/src/sandbox.ts

+        execId,
+        offset,
+        new Uint8Array(0),
+        true,


modal-js/test/sandbox.test.ts

saltzm · 2025-12-16T20:03:41Z

modal-js/test/task_command_router_client.test.ts

+  await expect(
+    callWithRetriesOnTransientErrors(func, 100, 2, null, deadline),
+  ).rejects.toThrow("Deadline exceeded");
+});


Would be good to have testing parity with python client

modal-js/src/task_command_router_client.ts

saltzm · 2025-12-16T20:14:25Z

modal-js/src/task_command_router_client.ts

+
+        try {
+          for await (const item of stream) {
+            numAuthRetries = 0;


Just changed this to a bool in the python implementation: https://github.com/modal-labs/modal-client/pull/3828/changes

Changed here too now to match!

saltzm

A few questions that weren't answered from the last review, but after that looks good to me

modal-js/test/sandbox.test.ts

saltzm · 2025-12-17T20:07:31Z

modal-js/src/sandbox.ts

+      this.#taskId,
+      this.#execId,
+      this.#deadline,
+    );


Noting that to match the python client we'd need to catch this deadline exceeded exception, but i actually like this behavior better of just throwing so think it makes sense to keep it

saltzm

One remaining question then looks good

saltzm · 2025-12-17T20:44:00Z

modal-js/src/sandbox.ts

+    stdoutConfig = TaskExecStdoutConfig.TASK_EXEC_STDOUT_CONFIG_DEVNULL;
+  } else {
+    throw new Error(`Unsupported stdout behavior: ${stdout}`);
+  }


Ah - I meant stdout = "stdout" 😄 There's a third option in the python client: https://github.com/modal-labs/modal-client/blob/efe8b48bbd66ba204447de2f035614870dfb1a6a/modal/stream_type.py#L12

Not adamant we support it here, I don't think it's commonly used, though a couple customers do use it. It just prints everything from the exec stdout to local stdout

saltzm · 2025-12-18T16:18:04Z

modal-js/test/sandbox.test.ts

+
+  const p = await sb.exec(["sleep", "999"], { timeoutMs: 1000 });
+  const t0 = Date.now();
+  const exitCode = await p.wait();


I think I'd expect this to throw based on the implementation? https://github.com/modal-labs/libmodal/pull/238/changes#diff-c6c555742ea2730b4a75182c097f0e07c70d86ec2af2b6f2a97bcb790c9777b7R96

Do you understand why it's not?

Ah - I meant stdout = "stdout" 😄 There's a third option in the python client: https://github.com/modal-labs/modal-client/blob/efe8b48bbd66ba204447de2f035614870dfb1a6a/modal/stream_type.py#L12

Not adamant we support it here, I don't think it's commonly used, though a couple customers do use it. It just prints everything from the exec stdout to local stdout

Ah ok, yeah these SDKs never supported that. Not entirely sure way, I'll look into it and might follow up in a separate PR!

Do you understand why it's not?

I believe it's because it's killed by the server-side timeout? The JS SDK returns a different returncode in that case though (Python returns 137 I believe). Should we align that behavior?

I think the python stdout API is a little weird IMO, so wouldn't hate not porting it.

I believe it's because it's killed by the server-side timeout?

Ah right. Wonder if it's non-deterministic though. Wouldn't be surprised if this test flakes if the client side hits the deadline first

Re: Aligning the returncode, IMO it would make more sense for wait() to just throw an exception that indicates a timeout - I wish that's what the python client did. But we broadly need better APIs for sandboxes for determining difference between killed w a signal, returning a code, timing out, etc.

ehdr added 2 commits December 16, 2025 18:28

Bump modal-client submodule and update protos

79329a0

Implement sandbox exec through command router

622cf1c

Port modal-labs/modal-client@6133e091 to add direct worker connections for Sandbox exec operations, reducing latency and improving reliability.

ehdr force-pushed the ehdr/sb-direct-exec branch from c947908 to 622cf1c Compare December 16, 2025 17:28

ehdr requested review from danielnorberg, freider, saltzm and thomasjpfan December 16, 2025 17:32

saltzm reviewed Dec 16, 2025

View reviewed changes

ehdr added 2 commits December 17, 2025 10:53

Don't initialize ignored streams

e6c88d4

Boolean check for retries

c6f670b

ehdr mentioned this pull request Dec 17, 2025

sandbox: dynamic directory mount & snapshot #239

Merged

More Sandbox exec tests

e7ee721

saltzm reviewed Dec 17, 2025

View reviewed changes

ehdr added 5 commits December 17, 2025 21:12

Remove unused returncode on ContainerProcess

e56e14f

Remove the legacy exec paths through server

a09393a

Test timeouts

c8a7ca5

Updage CHANGELOG

c97ab70

Don't initialize ignored streams, revisited

4580ac9

saltzm approved these changes Dec 18, 2025

View reviewed changes

ehdr merged commit dfef157 into main Dec 19, 2025
6 checks passed

ehdr deleted the ehdr/sb-direct-exec branch December 19, 2025 13:41

ehdr mentioned this pull request Dec 23, 2025

Implement sandbox exec through command router - Go #242

Open

Conversation

ehdr commented Dec 16, 2025 • edited by cursor bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saltzm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saltzm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saltzm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saltzm Dec 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ehdr commented Dec 16, 2025 •

edited by cursor bot

Loading

saltzm Dec 19, 2025 •

edited

Loading