Skip to content

fix(runtime): unblock canceled MicroSandbox input streams - #1467

Merged
skevetter merged 4 commits into
mainfrom
codex/microsandbox-logs-cancel-parity
Oct 13, 2026
Merged

skevetter merged 4 commits into
mainfrom
codex/microsandbox-logs-cancel-parity

Conversation

@skevetter

@skevetter skevetter commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

Canceled MicroSandbox commands could remain blocked while copying an owned input stream, even after the child process exited. Agent binary transfer had the same problem because its progress wrapper hid the source closer. Close eligible non-file input on cancellation, join the callback before returning, and close each acquired transfer source exactly once so retries receive a fresh source. Normal completion, failed starts, caller-owned files, existing transfer limits, and original cancellation causes retain their behavior.

Add six actual-VM scenarios across the built-in driver and published external provider v0.1.7: completed stdout/stderr logs while running and stopped, cancellation of a ready parent/child tree, and cancellation with blocked owned stdin. Independent PID/start-time/nonce observation checks process death within the same ten-second cancellation bound; VM creation time, boot ID, subsequent command execution, and source contents must survive. The external test path uses the real self-executed runtime supervisor and prepared executable. Each cleanup owns only its recorded process or previously absent VM identity.

The initial native run passed all four cancellation cases and the preceding 58 VM scenarios. Both logs assertions found the markers joined to earlier binary SSH output without a line boundary. Give each marker a leading newline as well as its trailing newline, retaining exact-line assertions, both channels and all existing deadlines. An owned shell replay rejects the old framing and accepts the corrected framing; the final-head CI run now passes actual running/stopped logs.

Validation:

  • Old production fails the new blocked-input and transfer-idle regressions; fixed focused race tests pass, including ownership, joining, fresh retries and original causes.
  • Final affected-package uncached race, vet, strict lint, all 13 pre-commit hooks, module verification, formatting and diff checks passed. The six-case Ginkgo dry run verified selection; the final-head CI now proves all six new actual VM cases.
  • Integrated the completed Dockerless slice. Fresh full committed local CodeRabbit reviewed all eight final PR files with zero findings, including the framing follow-up. All 81 final-head implementation CI jobs passed, including all 64 MicroSandbox VM scenarios (58 existing and six new). Fresh Greptile completed at 5/5 with no actionable issues. Full CodeRabbit reviewed all eight files with no actionable findings, minimal merge risk and no architecture-level concerns. The private-helper documentation warning and precautionary shared-stream proposal were independently dispositioned. Commit signatures are GitHub-valid; actionable threads are clear.

This does not establish TTY or arbitrary nonclosable-input cancellation, binary log fidelity or ordering, runtime compatibility failures, invalid-recreate preservation, host hardening, or existing-workspace migration. Built-in-provider cutover remains separately gated.

…gs-cancel-parity

# Conflicts:
#	.github/workflows/pr-ci.yml
#	sites/docs-devsy-sh/content/docs/developing-providers/runtime-protocol.mdx
@netlify

netlify Bot commented Oct 11, 2026 •

Copy link
Copy Markdown

✅ Deploy Preview for devsydev canceled.

Name Link
🔨 Latest commit b4b4135
🔍 Latest deploy log https://app.netlify.com/projects/devsydev/deploys/6acdc38a54cf969b8c95fe14

@coderabbitai

coderabbitai Bot commented Oct 11, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 78a68cb5-50fb-4c4b-8a82-ba947c06bd83
📥 Commits

Reviewing files that changed from the base of the PR and between ada5059 and b4b4135.

📒 Files selected for processing (8)
  • .github/workflows/pr-ci.yml
  • e2e/e2e_suite_test.go
  • e2e/tests/up/provider_microsandbox_logs_cancel.go
  • pkg/agent/delivery/exec_stream_cancellation_unix_test.go
  • pkg/agent/delivery/kubernetes.go
  • pkg/driver/microsandbox/cliclient.go
  • pkg/driver/microsandbox/exec_cancellation_unix_test.go
  • sites/docs-devsy-sh/content/docs/developing-providers/runtime-protocol.mdx

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 0 remain after this review.

📝 Walkthrough

This change adds a Microsandbox logs-and-cancellation integration scenario. It updates cancellation cleanup for eligible Exec input streams and agent transfer sources, adds focused tests, and documents the scenario and its coverage boundaries.

Changes: Microsandbox logs and cancellation

Layer / File(s) Summary
Exec input cancellation
pkg/driver/microsandbox/cliclient.go, pkg/driver/microsandbox/exec_cancellation_unix_test.go
Exec closes eligible non-file io.ReadCloser input on cancellation and waits for an active close callback. Unix tests cover blocked input, ownership, process-start failure, and already-canceled contexts.
Agent transfer source cleanup
pkg/agent/delivery/kubernetes.go, pkg/agent/delivery/exec_stream_cancellation_unix_test.go
Each transfer attempt uses a source wrapper that closes the underlying source at most once. Cancellation closes the source, and tests cover retries, exhausted retries, and normal completion.
Logs and cancellation E2E coverage
e2e/e2e_suite_test.go, e2e/tests/up/provider_microsandbox_logs_cancel.go, .github/workflows/pr-ci.yml, sites/docs-devsy-sh/content/docs/developing-providers/runtime-protocol.mdx
The E2E scenario checks logs while running and stopped, cancellation of ready and blocked-input processes, process termination, and workspace invariants. The test binary dispatches runtime-supervisor invocations to cmd.Execute(). CI and documentation include the scenario and its coverage boundaries.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant cliClientExec
  participant OwnedStdin
  Caller->>cliClientExec: Exec with context and stdin
  Caller->>cliClientExec: Cancel context
  cliClientExec->>OwnedStdin: Close input
  cliClientExec-->>Caller: Return after close callback completes
Loading
Merge Risk: ⚪ Minimal · up to b4b41

No identified issue blocks merging this cancellation and logs change after normal checks.

Security architecture risk: 🔵 Low · up to b4b41

Cleanup is scoped to supplied streams and individual transfer attempts, with protections for caller-owned files, failed starts, and fresh retries. Remaining risk concerns custom streams whose closure could block or affect shared resources; no such security violation was established.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The new authority acts on the supplied execution-input object or the source acquired by an individual delivery attempt. Broader impact would depend on that object's implementation or sharing. Guest root execution for literal-argv commands already existed and is not newly granted by this cleanup change.

Trust Boundaries and Controls

  • observed — Exec arms input closure only after successful process startup, excludes direct os.File inputs, and operates on the same object supplied by the caller. Closing supplied input is also an existing external-runtime convention, providing counterevidence to treating closure alone as a new trust-boundary violation.

Resilience and Maintainability Implications

  • observed — Delivery's sync.Once wrapper makes cancellation and owner cleanup converge on one underlying Close. Joining prevents cleanup from outliving return, but does not impose a time bound on arbitrary Close implementations; the documented contract requires Close to unblock Read.

Hardening Proposals

  • proposed — Document cancellation-time ownership and Close requirements alongside the shared stdin interfaces. If independently shared streams must be supported, consider explicit opt-in ownership rather than inferring it from io.ReadCloser. This is precautionary, not an established shared-resource defect.
Pre-merge checks · Passed 4 · Failed 1

Failed Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 18 functions across 6 files. (2 skipped: 2… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks (4 passed)
Check name Status Explanation
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly describes the primary runtime fix: unblocking canceled MicroSandbox input streams.
Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 18 functions across 6 files. (2 skipped: 2 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches
✨ Simplify code
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@netlify

netlify Bot commented Oct 11, 2026 •

Copy link
Copy Markdown

✅ Deploy Preview for images-devsy-sh canceled.

Name Link
🔨 Latest commit b4b4135
🔍 Latest deploy log https://app.netlify.com/projects/images-devsy-sh/deploys/6acdc38ad1806d8594af88a9

@skevetter

Copy link
Copy Markdown
Contributor Author

@greptileai review

@greptile-apps

greptile-apps Bot commented Oct 13, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

[High impact] The PR appears safe to merge; no actionable defects were found.

Summary

The PR unblocks canceled MicroSandbox commands by closing eligible input and waiting for the close callback. Agent delivery separately closes each acquired source once before retrying with a fresh source.

  • MicroSandbox commands can finish after blocked input is canceled.
  • Canceled binary transfers release their source before retrying.
  • Both MicroSandbox providers now have real-VM checks for logs and cancellation.

No actionable issues were found. This review did not run the tests; actual VM verification remains pending as stated by the author.

skevetter explicitly excludes binary log fidelity, ordering, TTY cancellation, and arbitrary nonclosable input from this PR. These limits are also documented.

Diagram

sequenceDiagram
  participant Caller
  participant Exec as MicroSandbox Exec
  participant Source as Eligible input
  participant Child as msb process
  Caller->>Exec: Start command
  Exec->>Child: Start and copy input
  Caller->>Exec: Cancel context
  par Stop process
    Exec->>Child: Kill through CommandContext
  and Unblock input
    Exec->>Source: Close
    Source-->>Exec: Read returns
  end
  Exec->>Exec: Wait and join close callback
  Exec-->>Caller: Return
Loading

Reviews (1) · Last reviewed commit: "test(runtime): frame MicroSandbox log ma..." · Reviewed by Greptile

@skevetter

Copy link
Copy Markdown
Contributor Author

The native-verification sentence in the review summary is stale. For exact head b4b41358e17589ceb5e5bc8d2416074dee7e405e, CI run 38352326977 passed all 81 implementation jobs. All ten MicroSandbox suites ran with the required runtime enabled: 58 existing cases and all six new logs/cancellation cases passed against the builtin and published external provider.

The logs/cancellation job reports 6 Passed | 0 Failed in 107.798 seconds. I inspected the complete native logs and updated the PR description. Binary log fidelity/ordering, TTY and arbitrary nonclosable-input behavior remain outside this change's claims.

@skevetter

Copy link
Copy Markdown
Contributor Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Oct 13, 2026 •

Copy link
Copy Markdown
✅ Action performed

Full review finished.

@skevetter

Copy link
Copy Markdown
Contributor Author

I inspected the complete eight-file review, including the retained security architecture and merge-risk assessment. No actionable defect or architecture-level concern remains. The supplied-stream ownership and Close-unblocks-Read requirement are intentional: direct caller-owned files and failed starts remain protected, delivery owns each acquired source, and cleanup joins before return. Arbitrary blocking Close implementations and independently shared input are outside the established contract; the precautionary interface-documentation proposal does not establish a defect in this change.

The documentation-coverage warning counts private helpers/tests and unsupported functions. Existing comments explain non-obvious ownership and joining invariants; adding comments solely to repeat helper names would not improve this contract. All 81 implementation CI jobs, all 64 actual MicroSandbox VM cases, Greptile 5/5 and the completed full CodeRabbit review cover exact head b4b41358e17589ceb5e5bc8d2416074dee7e405e.

@skevetter
skevetter marked this pull request as ready for review October 13, 2026 07:17
@skevetter
skevetter merged commit cd5bd93 into main Oct 13, 2026
103 checks passed
@skevetter
skevetter deleted the codex/microsandbox-logs-cancel-parity branch October 13, 2026 07:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant