fix(integrations): make tether connect down actually stop the process#232
Open
rylinjames wants to merge 1 commit into
Open
fix(integrations): make tether connect down actually stop the process#232rylinjames wants to merge 1 commit into
tether connect down actually stop the process#232rylinjames wants to merge 1 commit into
Conversation
Audit §3.9 (H3/H4/M7/M8/M9/L8/L11). - H3 (the headline): `connect up` Popened a child, stored the handle in a module-level dict, then the CLI process exited — orphaning the child. A later `connect down` ran in a NEW process with an empty dict, found nothing, and returned "external_still_running" while the GPU process leaked. Now connect persists the pid to ~/.tether/integrations/<name>.pid (honoring TETHER_HOME) and disconnect signals that pid across invocations, escalating SIGTERM → SIGKILL, and cleans up stale pid files. - M7: is_installed() used importlib.import_module — which EXECUTES the target's import-time side effects (rtsm[gpu] can init CUDA) just to answer "installed?". Switched to importlib.util.find_spec (no execution) + an explicit import_name field for packages whose import name != pip name. - M8/M9: start() piped child stdout/stderr and never read them → a chatty child fills the 64KB pipe buffer and blocks (looks like a hang). Now redirects to ~/.tether/integrations/<name>.log, and polls proc.poll() each iteration so a crash-on-launch fails fast with the log tail instead of waiting the full 30s. - H4 (partial): the hardcoded cu128 PyTorch wheel index is now applied only on Linux + a cu* extra (it's wrong on macOS/Jetson/CPU); added an optional pip_version_spec so a load-bearing integration can be pinned. The interactive install-consent prompt needs a new CLI flag and is left for a separate change. - L8: disconnect honors integration.stop_signal. L11: connect/disconnect are now exported from the package. Tests (tests/test_integrations_lifecycle.py, 7): cross-process pid stop, stale pid cleanup, find_spec install-check (+ explicit import_name), fail-fast start, version-pinned pip_spec, exports. Existing test_integrations.py still passes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Audit §3.9 (H3/H4/M7/M8/M9/L8/L11).
H3 — the headline:
tether connect downwas a structural no-opconnect upPopened the integration, stored the handle in a module-level dict, then the CLI process exited — orphaning the child. A laterconnect downran in a new process with an empty dict, found nothing, and returnedexternal_still_runningwhile the GPU process leaked.Now
connectpersists the pid to~/.tether/integrations/<name>.pid(honoringTETHER_HOME);disconnectsignals that pid across invocations, escalates SIGTERM → SIGKILL, and cleans up stale pid files.Other fixes
is_installed()usedimport_module— which executes the target's import-time side effects (rtsm[gpu] can init CUDA) to answer "installed?". Nowimportlib.util.find_spec(no execution) + an explicitimport_namefield for packages whose import name ≠ pip name.start()piped child output and never read it → 64KB pipe-buffer deadlock (looks like a hang). Now redirects to~/.tether/integrations/<name>.logand pollsproc.poll()each iteration so a crash-on-launch fails fast with the log tail instead of waiting 30s.cu128PyTorch wheel index is applied only on Linux + acu*extra (wrong on macOS/Jetson/CPU); added optionalpip_version_specto pin load-bearing integrations.disconnecthonorsstop_signal;connect/disconnectare exported.Tests
tests/test_integrations_lifecycle.py(7): cross-process pid stop, stale-pid cleanup, find_spec install-check (+ explicit import_name), fail-fast start, version-pinned spec, exports. Existingtest_integrations.pystill passes.Deferred
The interactive install-consent prompt (H4a) needs a new CLI flag (
--yes) — left for a separate change since it's customer-facing API naming.🤖 Generated with Claude Code