Skip to content

fix: skip signal handlers outside main thread (#136)#138

Open
raywcm wants to merge 2 commits into
coreweave:mainfrom
raywcm:fix/worker-thread-cleanup-import
Open

fix: skip signal handlers outside main thread (#136)#138
raywcm wants to merge 2 commits into
coreweave:mainfrom
raywcm:fix/worker-thread-cleanup-import

Conversation

@raywcm

@raywcm raywcm commented Jun 9, 2026

Copy link
Copy Markdown

Closes #136

Summary

Importing cwsandbox from a worker thread currently fails because _cleanup.py installs SIGINT/SIGTERM handlers at import time, and signal.signal() is only valid in the main thread.

This keeps the atexit cleanup registration available from any thread, but skips installing signal handlers when _install_handlers() runs outside the main thread. If the module is later initialized from the main thread, the signal handlers can still be installed.

Verification

  • Added unit coverage for worker-thread handler installation.
  • Confirmed a worker-thread import of cwsandbox._cleanup no longer raises ValueError.
  • Ran pytest tests/unit/cwsandbox/test_cleanup.py -q: 18 passed.
  • Ran python -m ruff check src/cwsandbox/_cleanup.py tests/unit/cwsandbox/test_cleanup.py: all checks passed.

@raywcm
raywcm requested a review from a team as a code owner June 9, 2026 08:37

@brandonrjacobs brandonrjacobs 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.

Thanks for fixing the crash — the main-thread guard is the right call given signal.signal()'s constraint. One gap worth mentioning though:

The signal handlers are never recovered after a worker-thread-first import.

_install_handlers() runs only once, at import (_cleanup.py:135), and nothing else
  in src/ ever calls it again. So in the exact scenario this PR targets — first import cwsandbox happening on a non-main thread — the signal handlers are
  skipped and stay skipped for the life of the process. A re-import from the main thread is a cached no-op and doesn't re-run the install.

Concretely: a later SIGTERM (e.g. a k8s/orchestrator shutdown) terminates the process without running _cleanup — atexit doesn't fire on signal-induced
termination — so sandboxes get orphaned and keep consuming compute. That's the guarantee this module exists to provide.

I verified this with a runtime repro of the control flow: after a worker-thread install, _handlers_installed=False and SIGTERM/SIGINT are still at default
disposition; only an explicit main-thread call to _install_handlers() ever wires them up.

Two smaller notes:

  • The PR description says handlers "can still be installed" from the main thread later — that recovery path doesn't actually exist in the code.
  • The skip is logged at debug, so this degradation is silent. Worth bumping to warning.

Suggestion: the root cause is doing main-thread-only work (signal.signal) as an import-time side effect. Rather than installing at import, make installation lazy + idempotent and trigger it from the first real SDK use (e.g. _LoopManager.get() / Sandbox.run()). That attempts install on the main thread whenever the SDK is actually used, which delivers the recovery this PR describes — and ends up being less code than the current flag-juggling. Happy to discuss.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Import time signal handler install crashes from non-main threads such as marimo

2 participants