Skip to content

fix: stop sandbox export children reliably on termination - #10891

Merged
manzt merged 2 commits into
mainfrom
codex/fix-export-termination-cleanup
Sep 18, 2026
Merged

manzt merged 2 commits into
mainfrom
codex/fix-export-termination-cleanup

Conversation

@Light2Dark

@Light2Dark Light2Dark commented Sep 18, 2026

Copy link
Copy Markdown
Member

This pull request was authored by a coding agent.

📝 Summary

Closes MO-7679

SIGTERM during sandbox export startup could interrupt Popen after the child started but before the caller received its handle, leaving the child running. Queue task cancellation so process creation finishes before cleanup reaps the child.

The handler preserves the first termination signal and exit code, allows cleanup to finish through repeated signals, and prevents queued cancellation from affecting work after the export scope closes. Deterministic tests cover interruption during process creation, termination at completion, and repeated termination during cleanup.

Validation:

  • The startup regression fails before the fix and passes with it on Python 3.10.
  • Both subprocess test modules pass on Python 3.10: 19 tests, including the original CLI cancellation cases.
  • Ruff lint and formatting checks pass.
  • Full checks were attempted in the isolated workspace but could not pass: frontend code generation rejected --experimental-transform-types, mypy reported three errors in unchanged tutorial code, and the sandbox blocked Pixi cache access.

📋 Pre-Review Checklist

  • For large changes, or changes that affect the public API: this change was discussed or approved through an issue, on Discord, or the community discussions (not applicable: internal bug fix).
  • Any AI generated code has been reviewed line-by-line by the human PR author, who stands by it.
  • Video or media evidence is provided for any visual changes (not applicable).

✅ Merge Checklist

  • I have read the contributor guidelines.
  • Documentation has been updated where applicable, including docstrings for API changes (no public API changes).
  • Tests have been added for the changes made.

Written by GPT-6 on Codex.

@vercel

vercel Bot commented Sep 18, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
marimo-docs Ready Ready Preview Sep 18, 2026 2:21pm UTC

Request Review

@github-actions

Copy link
Copy Markdown
Contributor

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All reported issues were addressed across 4 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread tests/_environments/test_process.py Outdated
@Light2Dark Light2Dark added the bug Something isn't working label Sep 18, 2026
@manzt
manzt marked this pull request as ready for review September 18, 2026 14:11
Copilot AI lite review requested due to automatic review settings September 18, 2026 14:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The changes directly address the described subprocess-ownership race and add focused tests that exercise the new termination and cleanup behavior.

Pull request overview

This PR fixes a race during sandboxed exports where receiving a termination signal (e.g., SIGTERM) while a subprocess is being spawned could interrupt ownership/cleanup and leave child processes running. It shifts export termination handling from “raise immediately in the signal handler” to “cancel the active export task and raise on scope exit,” ensuring the subprocess is fully owned and can be reliably reaped.

Changes:

  • Update export termination handling to queue task cancellation and preserve the first received termination signal until cleanup completes.
  • Harden run_command pipe-reader startup so partial reader setup failures still trigger subprocess cleanup and pipe draining.
  • Add targeted tests covering pipe-reader setup failure, termination at completion, termination during process creation, and repeated termination during cleanup.
File summaries
File Description
tests/_environments/test_process.py Adds a regression test ensuring reader-start failures still trigger subprocess cleanup and drain any started readers.
tests/_cli/test_export_subprocess.py Adds deterministic termination-race tests (completion-time signals, repeated termination, and interruption during Popen creation).
marimo/_environments/process.py Tracks created pipe-reader futures so cleanup can drain whatever successfully started, even if later setup fails.
marimo/_cli/export/_common.py Implements export termination via queued task cancellation + deferred SystemExit, preserving signal code and preventing cancellation leakage after scope exit.
Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

The SIGTERM race is fixed by deferring termination to task cancellation,
so Popen can return before cleanup takes ownership of its child.

Leave pipe-reader startup failures to a separate change. Keep the signal
regressions and existing CLI cancellation coverage.
@manzt
manzt merged commit 43c1089 into main Sep 18, 2026
48 of 49 checks passed
@manzt
manzt deleted the codex/fix-export-termination-cleanup branch September 18, 2026 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working team-draft

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants