Skip to content

fix(sandbox): complete successful main processes - #2884

Merged
drew merged 12 commits into
mainfrom
codex/2879-completed-main-process
Aug 29, 2026
Merged

fix(sandbox): complete successful main processes#2884
drew merged 12 commits into
mainfrom
codex/2879-completed-main-process

Conversation

@drew

@drew drew commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator

Summary

Restore ephemeral canonical-command semantics for sandbox create. The canonical main process controls the sandbox lifecycle, and foreground create streams output and returns the command exit code.

Related Issue

Closes #2879

Changes

  • Add the public Completed sandbox phase across protobuf, SDK, CLI, and TUI surfaces.
  • Map exit code 0 to Completed/MainProcessCompleted.
  • Map nonzero and signal-normalized exits to Error/MainProcessFailed, preserving the exact exit code. For example, SIGINT is reported as 130.
  • Keep explicit sandbox stop as Stopped/SandboxStopped.
  • Distinguish infrastructure errors from command failures by condition reason and the absence of a main-process exit result.
  • Declare foreground attachment intent only for attached CLI creates; detached and SDK/TUI workloads exit immediately. Keep the supervisor relay available until the peer SSH channel closes, with no terminal-drain timeout.
  • Send the SSH exit result only after the gateway durably records the terminal phase and exit code.
  • Make explicit create commands foreground by default; --detach remains available for long-running workloads.
  • Delete --no-keep sandboxes only after stdout, stderr, and the command result have drained.
  • Preserve restart and stale-instance fencing for retained terminal sandboxes.
  • Update lifecycle tests, architecture documentation, user documentation, and the OpenShell CLI skill.

Testing

  • mise run pre-commit
  • cargo test -p openshell-server --lib -q — 1,415 passed, 8 ignored
  • OPENSHELL_E2E_DOCKER_TEST=sandbox_lifecycle mise run e2e:docker — 7 passed
  • Supervisor and CLI lifecycle tests
  • Python type checking and tests — 89 passed
  • mise run sdk:ts:ci
  • Go SDK tests and generated-protobuf checks
  • GitHub branch checks on Linux amd64, Linux arm64, and macOS

Checklist

  • Follows Conventional Commits
  • Commits are signed off for DCO
  • User-facing and architecture documentation updated
  • Sandbox infrastructure E2E exercised

Signed-off-by: Drew Newberry <anewberry@nvidia.com>
@github-actions

Copy link
Copy Markdown

Signed-off-by: Drew Newberry <anewberry@nvidia.com>
krishicks
krishicks previously approved these changes Aug 21, 2026
Signed-off-by: Drew Newberry <anewberry@nvidia.com>
@krishicks

Copy link
Copy Markdown
Collaborator

I'm trying out a new format for agent review. gpt-5.6-sol medium found some issues, which after some back and forth was reshaped into the following output, which I think is much more human-readable and understandable.


Gateway cleanup can preempt SSH exit delivery

Before ef29680, Ctrl-D ended an ordinary SSH shell. SSH delivered the shell’s exit status directly, while the sandbox remained Ready; no gateway lifecycle cleanup was triggered by that shell exit.

After ef29680, Ctrl-D ended the canonical main process. Its result was reported to the gateway, which transitioned the sandbox to terminal Error and could tear down its runtime connectivity.

In this PR, the supervisor attempts to coordinate durable result reporting with SSH delivery, but it marks the terminal event as “delivered” before actually sending the SSH exit status. It then waits for the gateway to acknowledge the result. That acknowledgement can trigger session cleanup and disconnect the relay before the pending SSH exit-status frame is sent. As a user, the command may finish and its result may be stored, but the CLI can see an interrupted SSH connection instead of the command’s exit status.

Details: crates/openshell-supervisor-process/src/ssh.rs:643-647.

Infrastructure failures can be presented as command failures

Before ef29680, the foreground command’s SSH exit status was the command result, while failures to create, connect to, or run the sandbox were separate CLI/transport errors. There was no durable canonical-main exit code on the sandbox record to confuse the two.

After ef29680, canonical-main exits and sandbox runtime failures both resulted in terminal Error. The stored condition could describe the cause, but the phase itself did not distinguish workload failure from infrastructure failure.

In this PR, the server introduces the intended distinction:

  • MainProcessFailed means the user’s command returned nonzero.
  • Other Error reasons mean the sandbox runtime or infrastructure failed.

However, an infrastructure Error may still be enriched with an observed exit_code, and the CLI currently treats any terminal Error containing an exit code as a main-process result. As a user, a sandbox runtime failure could therefore be reported as though their command simply exited nonzero, hiding the actual infrastructure error.

Details: crates/openshell-server/src/compute/mod.rs:3359-3369, :3398; crates/openshell-cli/src/run.rs:764-771.

SSH credentials can cross a sandbox runtime generation

Before ef29680, Ctrl-D ended only an SSH shell and did not create a new sandbox runtime generation. The SSH session could remain valid for its configured TTL, but reconnecting reached the same still-running sandbox.

After ef29680, Ctrl-D ended the canonical main process and left the sandbox terminal. Restarting an Error sandbox directly was rejected. To create a fresh runtime generation, the user had to stop and start the sandbox, and the stop path disconnected the supervisor and deleted the sandbox’s SSH sessions before restart.

In this PR, Completed and MainProcessFailed sandboxes can be started directly without passing through the stop cleanup path. As a result, an SSH credential issued for the previous runtime generation can remain valid against the newly started generation. As a user—or anyone holding that credential—the old credential can access a fresh workload instance that previously would have required newly issued credentials.

Details: pre-PR cleanup at ef29680^:crates/openshell-server/src/compute/mod.rs:1125-1137 and :2960-2967; new direct-start eligibility at crates/openshell-server/src/compute/mod.rs:1183-1194.

Signed-off-by: Drew Newberry <anewberry@nvidia.com>
@drew

drew commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the three findings in 9cc6de9:

  • Terminal delivery now uses a two-phase report: the gateway persists the result while deferring ephemeral cleanup, then cleanup is released only after the SSH exit-status/output send finishes or is cancelled.
  • The CLI now treats Error plus exit_code as a command result only when the Ready=False/MainProcessFailed condition identifies a main-process failure; infrastructure errors remain infrastructure errors.
  • Restarting a completed or main-process-failed sandbox now cleans up the prior supervisor and SSH sessions before starting the next runtime generation.

Added regression coverage across supervisor-process, sandbox sidecar transport, server compute lifecycle, and CLI integration. Local validation: mise run pre-commit; affected Rust package tests; Docker sandbox_lifecycle e2e (7/7); Go format/build/lint/docs/proto checks.

Signed-off-by: Drew Newberry <anewberry@nvidia.com>
@drew drew added the test:e2e Requires end-to-end coverage label Aug 28, 2026
@github-actions

Copy link
Copy Markdown

Label test:e2e applied for 595c432. Open the existing run and click Re-run all jobs to execute with the label set. The run will execute the standard E2E suite after building the required gateway and supervisor images once. The matching required CI gate status on this PR will flip green automatically once the run finishes.

@krishicks

Copy link
Copy Markdown
Collaborator

Agent review:


Potential Concerns

  • Finished detached commands remain publicly Ready for up to 30 seconds — Before this PR, a natural main-process exit was reported immediately. With this PR, every normal exit waits up to 30 seconds for an attachment, including --detach and SDK-created workloads. During that interval, the process is already finished and SSH operations are rejected, but waitReady can still return success and observers still see Ready. Automation can therefore receive a usable-looking sandbox that rejects connections, and completion watches are delayed. (crates/openshell-supervisor-process/src/run.rs:456)

  • The terminal-delivery fence still acknowledges enqueueing, not transport delivery — The earlier review found that cleanup could preempt SSH exit delivery. The new two-phase protocol improves ordering, but terminal_delivery_complete is marked when the russh::Handle calls return; those calls only enqueue messages into russh’s internal channel. The supervisor then releases ephemeral cleanup and aborts its relay. Under backpressure or a slow connection, queued output or the exit status can still be lost before reaching the client. (crates/openshell-supervisor-process/src/ssh.rs:796, crates/openshell-supervisor-process/src/ssh.rs:990, crates/openshell-supervisor-process/src/run.rs:486)

krishicks
krishicks previously approved these changes Aug 28, 2026

@krishicks krishicks left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't feel strongly about the agent feedback on this; would rather see this get merged sooner than later.

Signed-off-by: Drew Newberry <anewberry@nvidia.com>
drew added 2 commits August 28, 2026 17:03
Signed-off-by: Drew Newberry <anewberry@nvidia.com>
Signed-off-by: Drew Newberry <anewberry@nvidia.com>
@drew

drew commented Aug 29, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed in f4e2628, b7d9553, and 627ac18.

There is no longer a lifecycle/drain timeout:

  • Detached CLI creates and SDK/TUI creates do not declare a future foreground attachment, so a finished main process reports its exit and the supervisor exits naturally without waiting.
  • A foreground CLI create explicitly declares that one attachment is expected. The process exit and exact exit code are persisted immediately; the terminal-delivery session then remains alive until that SSH attachment starts and the peer closes its channel.
  • Terminal delivery is no longer considered complete when russh Handle calls merely enqueue frames. Finalization is fenced on the peer channel-close callback, after retained output and exit-status delivery, and only then can ephemeral cleanup run.
  • The old defer_ephemeral_cleanup request flag was removed rather than retained as a compatibility mode. Its protobuf field number/name are only reserved to prevent wire reuse.
  • The gateway now keeps both SSH-session creation and its ForwardTcp transport reachable during this terminal-delivery phase, removing the fast-command readiness race.

Verification:

  • mise run pre-commit
  • focused terminal relay/unit regression test
  • OPENSHELL_E2E_DOCKER_TEST=sandbox_lifecycle mise run e2e:docker — 7 passed, including fast zero/nonzero commands, disconnect/reconnect replay, and --no-keep cleanup.

drew added 2 commits August 28, 2026 17:34
Signed-off-by: Drew Newberry <anewberry@nvidia.com>
Signed-off-by: Drew Newberry <anewberry@nvidia.com>
drew added 2 commits August 28, 2026 17:44
Signed-off-by: Drew Newberry <anewberry@nvidia.com>
Signed-off-by: Drew Newberry <anewberry@nvidia.com>
@drew
drew merged commit 69a05eb into main Aug 29, 2026
74 checks passed
@drew
drew deleted the codex/2879-completed-main-process branch August 29, 2026 01:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test:e2e Requires end-to-end coverage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: support ephemeral job-style sandboxes

2 participants