feat(execd): add caller-bound command and PTY creation recovery - #1771
destire-mio wants to merge 10 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce5ee110b1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Read the OSEP and the runtime implementation end-to-end — reviewing from the agent-client perspective, since a lost create response producing a second process is exactly the #1547 failure mode autonomous clients hit in practice. The core contract looks right: mutex-scoped claim with owner-only launch, a typed fingerprint payload (map-key sorting, absent/empty env equivalence), identity-encoded expiry instead of tombstones, and creation state cleanly separated from execution state with honest unknown-outcome semantics on instance mismatch. The failure/recovery table in the guide is the clearest statement of the guarantee I've seen in this repo. Four thoughts, none blocking:
|
|
The renumber to 0024 and the follow-up in 2904c1d look great — the instance-snapshot cache (60s monotonic window, shared fetch on concurrent misses, invalidation on I read the new |
|
PTY operation state can never reflect a failed launch
The real launch happens later, on the first WebSocket connection, with exactly one attempt (
So a recovering caller that follows the documented flow (wait for Options:
(1) keeps PTY semantics uniform with commands; (3) is the cheap fix. |
|
Caller-bound status redaction is inconsistent and drops the only diagnostic for foreground commands
The trust boundary is identical in all three cases (any caller holding the execd token can query any handle), so this doesn't reduce exposure — it only makes the new path less diagnosable. The sharpest edge: for a foreground caller-bound command whose response was lost, the redacted Suggestions:
|
|
Heads-up: this branch currently conflicts with A trial merge against latest
Could you merge (or rebase onto) current |
Expose PTY launch outcome through session status, retain command launch errors under recovered handles, and preserve native argv semantics and SDK request compatibility when merging upstream main.
|
Friendly reminder: this branch currently conflicts with
Could you rebase onto the latest |
Pangjiping
left a comment
There was a problem hiding this comment.
Automated code review — Request Changes
Automated review (OpenCodeReview / glm-5.3) of 47ccb7af..a6ae1232 (84 files, +8394/−86): 28 findings — 1 critical, 1 high, 6 medium, 20 low.
Blocking issues:
- [CRITICAL] Go SDK build break —
sdks/sandbox/go/execution_operations.go:23,130imports stdlibmaps(Go 1.21+) whilego.modstill declaresgo 1.20and CI pinsgo-version: "1.20"(sdk-tests.yml:314). The module no longer compiles at its declared minimum toolchain;go-sdk-qualitywill fail. - [HIGH] Data race / copylocks —
sdks/sandbox/go/execution_operations.go:128-130copiesExecdClientby value (client := *e.client), duplicating an in-usestreamOnce sync.Onceand racing concurrent SSE callers onstreamClient. Pass per-request extra headers instead of cloning the Client.
The core execd recovery state machine (claim-before-launch, 202 creating, 409/410 semantics, retention/janitor, concurrency outside the registry mutex) held up well under review. The concentrated risk is SDK client plumbing: the two Go issues above, Windows hook ordering (MEDIUM, command stays creating), C#/Kotlin single-flight cancellation/failure handling, and Python adapters skipping the SDK-wide SandboxException error conversion in exactly the network-failure scenario this feature targets.
Individual findings are posted as inline comments below. Requesting changes until the critical build break, the data race, and the six medium findings are addressed.
|
Thanks @Pangjiping for the review. I addressed the findings in The fixes cover Go 1.20 compatibility and the client-copy race, Windows startup notification ordering, C# cancellation and argv support, Kotlin shared-fetch failure handling, and Python exception conversion, along with the validation and cleanup items. For the Kotlin empty-command finding, the existing private constructor and builder reject invalid requests before they reach the adapter. I added regression tests for those constraints. Local validation on the merge revision passed, including execd race tests, Go 1.20 tests, and the SDK suites listed in the PR description. Native Windows execution and deployment E2E remain unverified; GitHub test workflows show Could you take another look and approve the pending workflow runs? |
Merge upstream/main at f3950db. Retain command operation recovery coverage alongside the upstream C# stream completion checks. Validation: Go and execd builds, C# build, Kotlin source and test compilation, JavaScript typecheck and lint, and Python Ruff and Pyright passed. Runtime regression tests were not run in this revision.
Retain upstream command helpers, session error handling, set_env support, and background process-group fixes alongside caller-bound recovery. Resolve SDK conflicts and align compatibility fixtures with current APIs.
Summary
Related to #1547. If a command starts and its creation response is lost before the caller saves the execution ID, an ordinary retry starts another process. This PR adds opt-in caller-bound command and PTY creation: callers persist an operation identity and immutable request before sending; matching retries recover the original handle, and conflicting requests return 409.
/command/operations,/pty/operations, instance discovery and private operation lookup. Claim the identity in the runtime Controller before launch and return the reserved handle with202 creatingwhile creation is pending. Legacy command SSE and PTY creation responses retain their behavior; unsupported servers never fall back to a legacy creation request.argv, request matching, command diagnostics and authenticated status lookup. PTYs remain dormant until their first WebSocket connection and get one launch attempt per saved identity. Optionallaunch_attemptedandlaunch_failedstatus fields distinguish dormancy, launch failure and completion after a lost WebSocket response. Reconnect, replay and takeover retain the original process.--operation-capacity/EXECD_OPERATION_CAPACITY(default 4096); expose bounded metrics through existing OpenTelemetry.f3950db2: preserve runtime-init gating, bind operation lookup to the authenticated runtime credential, return structured recovery errors withCache-Control: no-store, retain Go response-header capture alongside request-local headers, and move execd documentation to the new architecture directory.The guarantee is at-most-once creation for the saved identity within one authenticated principal, resource kind and execd Controller lifetime. The window starts at the server timestamp encoded in the identity. Expired identities return 410; instance mismatch returns 409 with unknown outcome. Callers sharing an access token share its trust boundary. Command handle recovery does not replay foreground output; callers needing retained logs choose background mode before creation.
This is an experimental implementation pending maintainer design review. Memory registration and OS spawn are not transactional. Cross-controller-restart recovery, command success and exactly-once business effects are outside the guarantee.
The September 23 review is addressed in
547f082f: Go 1.20 compatibility and client-copy race, Windows startup notification ordering, C# cancellation/argv/response handling, Kotlin shared-fetch failure completion, Python exception conversion, input validation and diagnostics, browser-compatible random IDs, and OpenAPI cleanup. For the Kotlin empty-command finding, the private constructor and existing builder already reject missing/empty command and invalid argv combinations; regression tests cover those constraints without adding an unreachable adapter branch.Testing
Validation of current merge revision
85f1a97643c73932473dc2b22e3159ffd4ba6282on macOS arm64 (September 26, Asia/Shanghai):go test -mod=readonly -race -count=1 -timeout=4m ./pkg/runtime ./pkg/web/... ./pkg/flag ./pkg/telemetrypassed. This includes real child-process and HTTP recovery regressions, PTY launch/reconnect behavior, request-body bounds, capacity, runtime-credential isolation, and structured authentication/init errors.-race -count=1, full suite with Go 1.20.14, andgo vet ./...passed. The concurrent SSE/operation-lookup regression checks that operation identity headers stay on the corresponding requests.pnpm testregenerated the API types and built the ESM/CJS/type outputs before running the suite; generated tracked sources match the committed sources.:sandbox:test :code-interpreter:test --rerun-tasks, including API generation and compilation, with JDK 17.git diff --checkremained clean after validation. No additional source changes were needed for this follow-up.The build/static checks recorded in the current merge commit were performed on September 25; the runtime/test results above were rerun on September 26 and supersede the older
63067854test counts.Not run in this follow-up: native Windows execution, Linux container/privileged bwrap/eBPF checks, Kubernetes/Jupyter deployment E2E, production load, instruction-level crash injection, docs build, or C# tests on additional runtime targets. Retention tests use controlled clocks rather than a 24-hour soak. Hosted CI remains separate: the 12 pull-request test workflows are awaiting approval (
action_required, zero jobs); the local results above do not establish hosted CI success or maintainer approval.Breaking Changes
Recovery is opt-in through new methods and paths. PTY launch-status fields are optional for older-server compatibility; absent fields mean unavailable launch information. Wait for PTY operation state
createdbefore attaching.Checklist