fix(agent-runtime): kill npx MCP orphans outside claude pgid that hold stdout pipe open (#618)#620
Merged
Merged
Conversation
The canvas is inside v-if="voice.isActive.value" so canvasEl.value is null when onMounted fires. renderFrame() exits early without scheduling the next frame, killing the loop permanently. Replace onMounted initialization with watch(canvasEl) so the RAF loop starts when the canvas enters the DOM and stops when it leaves. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…d stdout pipe open (#618) After terminate_process_group kills claude's pgid, npm→node MCP server chains spawned via npx call setsid() and land in a new session — they survive the pgid kill and keep the stdout pipe write FD open indefinitely. The kernel cannot deliver EOF to our reader thread while any writer FD remains open, so drain_reader_threads blocked for the full 30s post_kill_grace, then lost the buffered result line via force-close (HTTP 502). Add _kill_orphan_pipe_writers(): after terminate_process_group, scan /proc/*/fd for any process outside our pgid that holds the pipe's write end (detected via fdinfo flags), and SIGKILL it. Killing the orphan releases all its FDs (stdout AND stderr write ends) simultaneously, delivering EOF to both reader threads so they drain naturally before the post_kill_grace window. New tests (Linux-only, skipped on macOS — /proc required): verify that a setsid() grandchild is detected and killed, that our own read-end process is not touched, and that the end-to-end drain path preserves buffered data. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
4 tasks
vybe
pushed a commit
that referenced
this pull request
May 23, 2026
* fix(tests): restore services.agent_client in sys.modules baseline (#762) test_voice_tools.py and test_fleet_status_resilience.py install incomplete stubs of services.agent_client into sys.modules at module-collection scope. The autouse restore in tests/conftest.py only restored keys whose baseline was non-None, and only ran for the non-unit tier (the unit tier sets norecursedirs = .. in tests/unit/pytest.ini and bypasses the parent conftest entirely). The polluted stubs missed CircuitState, so transitive importers (adapters → task_execution_service:32 → from services.agent_client import CircuitState) raised ImportError, taking down 19 tests in test_file_upload.py and 1 in test_session_persistence_flag.py. Two changes: 1. tests/conftest.py: add services.agent_client to _SYS_MODULES_INVARIANT_KEYS and pre-import it before the baseline is captured, so the baseline is a real module object that gets restored between tests (covers the non-unit tier). 2. tests/unit/conftest.py: mirror the baseline+autouse-restore mechanism for services and services.agent_client. The unit tier has its own rootdir; without this, the parent conftest's defenses never run for the unit suite. Unit tier: 20 failed → 4 failed (the 4 remaining are unrelated SQL schema and KeyError failures in test_extract_photo_largest_size and test_voice_auth, out of scope for this task). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(tests): narrow except in services.agent_client preload to ImportError Follow-up to #762 — code-review feedback. except Exception: pass would mask non-import errors (e.g. side-effect runtime exceptions at module load) and silently degrade the autouse-restore defense. ImportError is the only expected failure mode for the preload. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(tests): evict polluted services.task_execution_service stub in CB probe tests Seven tests in TestCircuitBreakerFastFail and TestCancelledErrorInExecuteTask failed with "object MagicMock can't be used in 'await' expression" — but only when test_validation.py ran first in the same pytest session. The TypeError fires at `await svc.execute_task(...)`, not on a single attribute mock. Root cause: test_validation.py does `sys.modules["services.task_execution_service"] = MagicMock()` at module-collection time. The conftest baseline-restore (#762) cannot undo it because the baseline value is `None` — the real task_execution_service module isn't loadable from conftest preload (it imports `database`, which mkdirs `/data` and fails outside Docker). The autouse restore explicitly preserves None-baseline entries to avoid clobbering deliberate stubs. When test_cb_probe later does `from services.task_execution_service import TaskExecutionService` it gets `MagicMock.TaskExecutionService`, instantiating returns a MagicMock, `svc.execute_task(...)` returns a MagicMock, and `await MagicMock` raises TypeError. Fix: in each affected class's autouse `_patch_env` fixture, pop `services.task_execution_service` from sys.modules before the test body's import — forcing a fresh load of the real module. Also replace the module-level `setdefault` of `utils.credential_sanitizer` with an unconditional install (test_validation.py installs a partial stub that lacks `sanitize_execution_log`, so setdefault was a no-op and the fresh task_execution_service import failed at `from utils.credential_sanitizer import sanitize_execution_log`). Verified: full file passes (10/10) standalone and after test_validation.py pollution. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(credentials): map agent-server connect errors to 503 on import/export Outcome (a) from the Task 5 plan: when an admin calls POST /api/agents/{name}/credentials/import on an agent whose container status is "running" but whose internal FastAPI server hasn't bound to port 8000 yet, `import_to_agent()` raises `httpx.ConnectError` ("All connection attempts failed"), which is not a `ValueError`. The existing handler had only `except ValueError → 400` and a bare `except Exception → 500`, so the transient race surfaced as a 500. `test_import_credentials_no_enc_file_fails` accepts 400 (file missing) or 503 (agent not ready) but never 500 — so the regression failed CI. Fix mirrors the pattern already used by `inject_credentials` (same file, line 250) and `routers/agent_files.py:82`: catch `httpx.RequestError` (parent of ConnectError / TimeoutException / ReadError) and map to 503 with a warning log. Applied symmetrically to `export_credentials` since it has the same shape and would 500 on the same transient condition. `CredentialsFileNotFoundError(ValueError)` is unaffected — when the agent server *is* reachable but no `.credentials.enc` exists, `import_to_agent()` still raises that ValueError subclass and the existing 400 mapping still fires. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(env): auto-load TRINITY_TEST_PASSWORD + REDIS_BACKEND_PASSWORD from .env Closes the env-friction set surfaced by the May 2026 test-recovery audit: - tests/setup-env.sh sourced by run-*.sh exports the four vars pytest needs from project .env. - tests/conftest.py picks them up the same way for direct pytest invocation (alias ADMIN_PASSWORD -> TRINITY_TEST_PASSWORD). - tests/requirements-test.txt installs src/cli editable so test_cli_admin_login.py and test_cli_profiles.py can collect. - tests/README.md documents the env matrix, tiers, the rate-limit + setup-completed recovery commands, and the Conductor-worktree backend-mount caveat. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(tests): override placeholder REDIS_BACKEND_PASSWORD with .env value The setdefault chain in tests/conftest.py was order-dependent: line 30 unconditionally setdefault'd "test" as a placeholder for backend-config import safety, which made the subsequent setdefault from .env a no-op. tests/security/ ACL tests then ran with the wrong password and failed with NOAUTH unless the caller manually exported REDIS_BACKEND_PASSWORD before pytest. Switch to explicit override: when the .env value is present AND the current env-var is either unset or the "test" placeholder, replace it. An explicit caller export (any value other than "test") still wins. Follow-up to dbdb808. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(lint): skip .venv/__pycache__ in sys_modules linter; regen baseline Two changes: 1. lint_sys_modules.py:iter_test_files — skip directories that contain third-party / generated code (.venv, venv, __pycache__, .pytest_cache, node_modules). The previous rglob walked into tests/.venv/lib/python3.11/site-packages and reported 30+ pseudo- violations in dependency code that vary per machine / dep version. The baseline already excluded these (zero .venv entries) so the committed baseline was machine-relative without anyone noticing until a dep upgrade exposed the drift. 2. tests/lint_sys_modules_baseline.txt — regenerate. Two real changes: - test_cb_probe_execution_close.py: 5 → 7 (+2 sys.modules.pop calls added in commit f586532 to evict cross-file stub pollution). - tests/unit/test_cleanup_unreachable_orphan.py removed (cleaned up during the dev rebase). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(tests): post-recovery report (May 2026 test-suite audit) Summary of the 7 fixes landed in this branch (CircuitState sys.modules, MagicMock-await eviction, credentials 503 mapping, env friction docs + helpers, lint baseline regen + linter .venv exclusion) plus an inventory of out-of-scope failures and Conductor-worktree caveats that need follow-up. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(subprocess-pgroup): regression test for #586 setsid pipe-holder Adds test_setsid_escapee_drained_via_orphan_killer_preserves_result_line to pin the full production path: parent emits result, forks a setsid()'d grandchild that holds stdout open, parent exits. Asserts that drain_reader_threads invokes _kill_orphan_pipe_writers and the buffered "RESULT_LINE" survives — i.e. the drain doesn't fall through to the force-close fallback that discards the kernel pipe buffer. Sibling of the #531 buffered-data test, distinct because the grandchild calls os.setsid() — the case that escapes terminate_process_group(pgid) and is the real-world signature in production (git push → ssh). Also adds a KNOWN_ISSUES.md entry describing the bug class, the platform fix (#620), and operator-side defense-in-depth for stop hooks that spawn network processes. Refs #586 Co-Authored-By: Claude <noreply@anthropic.com> * docs(feature-flows): backfill notes for #602/#830, 35d4e78, #759/#779 - agent-lifecycle.md / container-capabilities.md: document Phase 3c capability drop (#602 / PR #830, 2026-05-13) — SYS_PTRACE, MKNOD, NET_RAW, FSETID removed from FULL_CAPABILITIES (now 9 caps, was 13); constants extracted to capabilities.py so test_capability_set.py can import them stdlib-only. - credential-injection.md: document 503 mapping for import/export endpoints (commit 35d4e78, 2026-05-17) — httpx.RequestError now surfaces 503 instead of 500, mirroring inject and agent-files. - session-tab.md: update lock semantics for #779 — cold turns now serialised on session_lock:cold:{session_id} (previously short- circuited, letting two concurrent first POSTs race on update_cached_claude_session_id and orphan one JSONL inside the agent). - feature-flows.md: index entries for the above. Co-Authored-By: Claude <noreply@anthropic.com> * docs(security): CSO daily audit 2026-05-17 + diff report 2026-05-13 - cso-2026-05-17.{md,json}: daily full-phase audit (Phases 0-14). Verdict CLEAR at 8/10 confidence gate; one persistent MEDIUM (Dockerfile USER hardening — tracked since 2026-04-05). No new CRITICAL or HIGH findings. - cso-diff-2026-05-13.md: diff-mode report covering the working-tree changes for the #586 regression test and KNOWN_ISSUES.md entry. No vulns or secrets — docs + test-only, no production code path. Co-Authored-By: Claude <noreply@anthropic.com> * chore(.claude): bump submodule (DEVELOPMENT_WORKFLOW.md) Picks up the methodology toolkit head that adds DEVELOPMENT_WORKFLOW.md. Co-Authored-By: Claude <noreply@anthropic.com> * fix(tests): resolve -e src/cli path from repo root (CI green) pip resolves relative paths in requirements files against the current working directory, not the requirements file. `-e ../src/cli` (added in dbdb808) only worked when pip was invoked from `tests/`; CI runs from the repo root and got `../src/cli` -> outside workspace -> 6 pytest jobs + schema-parity + regression-diff all red on PR #875. Switch to `-e ./src/cli` and update tests/README.md to instruct invoking from repo root. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(tests): restore sys.modules["config"] after test_config_fail_fast reload test_config_accepts_url_with_credentials calls _reload_config() which pops sys.modules["config"] and re-imports. The reloaded module reads SECRET_KEY from os.environ at reload time, picking up "test-secret-key-for-unit-tests" from test_voice_auth.py's os.environ.setdefault — but the *original* config (loaded at conftest pre-import time, before that env was set) had a random SECRET_KEY from secrets.token_hex(32). The test then leaves the new module in sys.modules permanently. Downstream, routers/voice.py's runtime `from config import SECRET_KEY, ALGORITHM` (called from voice_websocket) reads the new SECRET_KEY, while test_voice_auth.py's JWTs were signed with the original key captured at its module-collection time. JWT decode raises JWTError, voice_websocket closes 4001 instead of the expected 4003/accept, and the 3 ownership-gate tests fail — but only under pytest-randomly seeds that schedule test_config_fail_fast before test_voice_auth (notably seed 12345 after PR #875 added test_subprocess_pgroup.py and shifted the random ordering). Snapshot/restore sys.modules["config"] via an autouse fixture so each test's reload is scoped to itself. The 3 voice_auth tests (test_owner_passes_auth_gate, test_other_user_rejected_4003, test_admin_bypasses_ownership) now pass on seed 12345. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(tests): use project-standard sys.modules restore helper in config_fail_fast Renames _restore_config_module to the lint-recognized _STUBBED_MODULE_NAMES + _restore_sys_modules pair so the helper-exception in tests/lint_sys_modules.py fires (precedent: tests/unit/test_telegram_webhook_backfill.py). The previous attempt (0966243) used a bespoke fixture name + bare sys.modules.pop, which is exactly what the #762 hygiene linter bans outside conftest.py — the lint job went red on push. Same behavior, correct pattern, exempted by the linter. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore(tests): baseline 6 pre-existing sys.modules mutations in test_slot_per_slot_ttl.py #871 landed on dev (commit 98574f3) with test_slot_per_slot_ttl.py containing 6 bare sys.modules.{pop,setdefault,assign} calls but no matching baseline entry. Dev's own post-merge lint job is now red on that commit (Issue #802 caught it as intended). Any PR that merges with current dev inherits the failure. This PR is unrelated to the slot file but blocked by the inherited red. Bump the baseline to match what dev actually ships, unblocking CI here. The proper fix — refactor test_slot_per_slot_ttl.py to use the _STUBBED_MODULE_NAMES + _restore_sys_modules helper pattern, or scope its sys.modules mutations via monkeypatch — should land as a follow-up on #871's author or whoever owns the slot file (file is otherwise untouched here). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Revert "chore(tests): baseline 6 pre-existing sys.modules mutations in test_slot_per_slot_ttl.py" This reverts commit 459b535. * chore(.claude): revert submodule regression to match dev The branch's submodule bump (3873d2c) was a parent of dev's current pointer (9650477), so merging would silently revert the "fix(announce): prevent duplicate Slack sends by switching to Python urllib" change in .claude. Resetting the submodule pointer to dev's current state effectively drops the bump from this PR. The DEVELOPMENT_WORKFLOW.md doc added by 3873d2c is still present (9650477 includes it as an ancestor). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: Eugene Vyborov <eugene@beingluminous.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.
Summary
npx -y <package>MCP servers are spawned by npm which callssetsid(), placing them in a new session/pgid outside claude's pgid. They surviveterminate_process_group(claude_pgid)and keep the stdout pipe write FD open indefinitely — the kernel never delivers EOF to our reader thread.terminate_process_group, scan/proc/*/fdinfofor any process outside our pgid that holds the stdout pipe's write end (inode match + O_WRONLY/O_RDWR flag check), and SIGKILL it. Killing the orphan releases all its FDs (stdout + stderr) simultaneously, delivering EOF to both reader threads so they drain naturally within thepost_kill_gracewindow.Changes
docker/base-image/agent_server/utils/subprocess_pgroup.py— add_kill_orphan_pipe_writers(), wire intodrain_reader_threadsbetweenterminate_process_groupand thepost_kill_gracejointests/unit/test_subprocess_pgroup.py— addTestKillOrphanPipeWriters(3 tests, Linux-only viaskipif—/procrequired; they run in Debian Docker containers)Test Plan
test_subprocess_pgrouptests passFixes #618
🤖 Generated with Claude Code