Skip to content

fix(langchain): avoid shared process-group kill in shell middleware#36359

Open
Yashodip More (yashodipmore) wants to merge 2 commits intolangchain-ai:masterfrom
yashodipmore:fix/shell-killpg-same-pgrp
Open

fix(langchain): avoid shared process-group kill in shell middleware#36359
Yashodip More (yashodipmore) wants to merge 2 commits intolangchain-ai:masterfrom
yashodipmore:fix/shell-killpg-same-pgrp

Conversation

@yashodipmore
Copy link
Copy Markdown
Contributor

@yashodipmore Yashodip More (yashodipmore) commented Mar 29, 2026

Fixes #36358


Summary
This PR fixes a process termination safety bug in ShellToolMiddleware where cleanup could kill the caller process group when create_process_group is False.

Why this change
When the shell child is started without a dedicated process group, it can share the parent group. The existing cleanup path used group kill unconditionally, which could terminate unrelated processes including the caller. This is a high-impact availability risk.

What changed

Updated shell session kill logic to use group kill only when the child is in a different process group than the caller.
Added safe fallback to child-only kill when both share the same process group.
Added regression tests for both scenarios:
Shared process group: no group kill, child-only kill.
Dedicated process group: existing group kill behavior is preserved.

Validation

Targeted new tests passed.
Full shell tool unit test file passed.

AI assistance disclosure
This contribution was prepared with assistance from an AI coding agent. I reviewed, validated, and finalized the proposed changes and test coverage before submission.

Areas for careful review

Process-group detection behavior on Linux and other POSIX environments.
Any implications for existing timeout and shutdown flows in shell middleware.
Whether additional integration-level coverage is desirable for process cleanup behavior.

@github-actions github-actions bot added fix For PRs that implement a fix langchain `langchain` package issues & PRs size: S 50-199 LOC labels Mar 29, 2026
@github-actions
Copy link
Copy Markdown

This PR has been automatically closed because you are not assigned to the linked issue.

External contributors must be assigned to an issue before opening a PR for it. Please:

  1. Comment on the linked issue to request assignment from a maintainer
  2. Once assigned, edit your PR description and the PR will be reopened automatically

Maintainers: reopen this PR or remove the missing-issue-link label to bypass this check.

@yashodipmore
Copy link
Copy Markdown
Contributor Author

Hi Mason Daugherty (@mdrxy),

Thanks for assigning the issue!

It seems my PR was auto-closed earlier due to the assignment policy. Now that I’m assigned, could you please confirm if I should reopen this PR or create a new one?

Happy to proceed either way and address any feedback quickly.

@github-actions github-actions bot reopened this Mar 30, 2026
@mdrxy Mason Daugherty (mdrxy) changed the title fix(langchain): avoid shared process-group kill in shell middleware (… fix(langchain): avoid shared process-group kill in shell middleware Mar 30, 2026
@eyurtsev
Copy link
Copy Markdown
Collaborator

Could you refine the PR summary a bit to provide appropriate context here. Does this affect Host Policy only? If so, This is a high-impact availability risk. is inaccurate as this middleware wouldn't be used in a web server setting. host policy is okay for a cli application, maybe a CI/CD pipeline (if appropriate steps can be taken for securing secrets etc)

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

Labels

external fix For PRs that implement a fix langchain `langchain` package issues & PRs size: S 50-199 LOC

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ShellToolMiddleware may kill caller process group when create_process_group=False

2 participants