Skip to content

fix(executor): terminate action process groups - #3159

Merged
daryllimyt merged 4 commits into
mainfrom
daryl/eng-1568-process-tree-containment
Jul 30, 2026
Merged

fix(executor): terminate action process groups#3159
daryllimyt merged 4 commits into
mainfrom
daryl/eng-1568-process-tree-containment

Conversation

@daryllimyt

@daryllimyt daryllimyt commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Start direct and unsafe action subprocesses in dedicated process sessions.
  • Terminate and reap the subprocess group as soon as its leader exits, or on timeout or cancellation, before the registry-artifact lease can be released.
  • Observe leader exit independently of stdout and stderr so a background descendant holding inherited pipes cannot delay cleanup.
  • Share the process-group communication and cleanup logic between the direct action runner and unsafe PID executor.

This is the correctness prerequisite for #3139: active action processes must be gone before their registry-artifact lease is released and the cache entry becomes eligible for eviction.

Scope

This contains ordinary descendants that remain in the action's process group. Deliberately detached sessions are outside the unsafe fallback's contract; full hostile-code isolation requires nsjail or PID-namespace support. Reaping orphaned grandchildren from a PID-1 worker is separate container-init hygiene because zombies cannot execute against or hold file descriptors into the leased artifact.

Testing

  • uv run pytest tests/unit/test_registry_artifacts.py tests/unit/test_action_runner.py tests/unit/test_unsafe_pid_executor.py -q (58 passed)
  • Post-restack direct and unsafe PID regression suite (31 passed)
  • Inherited-output-pipe regression repeated 20 times (20 passed)
  • Ruff check and format clean on all changed Python files
  • BasedPyright on all changed Python files (0 errors, 0 warnings)

LOC breakdown

Category + -
Logic 68 11
Tests 215 2

Related Issues

ENG-1568

daryllimyt commented Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

@daryllimyt daryllimyt added fix Bug fix security Security related issue labels Jul 30, 2026
@daryllimyt
daryllimyt marked this pull request as ready for review July 30, 2026 17:30
@daryllimyt
daryllimyt requested a review from jordan-umusu July 30, 2026 17:30
@zeropath-ai

zeropath-ai Bot commented Jul 30, 2026

Copy link
Copy Markdown

No security or compliance issues detected. Reviewed everything up to fe89081.

Security Overview
Detected Code Changes
Change Type Relevant files
Enhancement ► tracecat/executor/action_runner.py
    Switch to using communicate_process_group for process I/O and start_new_session for subprocesses
Enhancement ► tracecat/sandbox/unsafe_pid_executor.py
    Use communicate_process_group, add start_new_session, and include pid namespace related utilities
Enhancement ► tracecat/sandbox/utils.py
    Add terminate_process_group and communicate_process_group for handling process groups and timeouts

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7cbc86c20d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread tracecat/executor/action_runner.py Outdated
Comment thread tracecat/sandbox/utils.py
@daryllimyt
daryllimyt force-pushed the daryl/eng-1568-process-tree-containment branch from 7cbc86c to 18be338 Compare July 30, 2026 17:39

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 18be338ed0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread tracecat/sandbox/utils.py

@jordan-umusu jordan-umusu 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.

LGTM

Comment thread tracecat/sandbox/unsafe_pid_executor.py Outdated
Comment thread tracecat/executor/action_runner.py Outdated
@daryllimyt daryllimyt changed the title fix(executor): terminate action subprocess trees fix(executor): terminate action process groups Jul 30, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 93cfd41bec

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread tracecat/sandbox/unsafe_pid_executor.py Outdated
@daryllimyt
daryllimyt force-pushed the daryl/eng-1568-process-tree-containment branch from 93cfd41 to fe89081 Compare July 30, 2026 19:51
@daryllimyt
daryllimyt merged commit d819bd1 into main Jul 30, 2026
16 checks passed
@daryllimyt
daryllimyt deleted the daryl/eng-1568-process-tree-containment branch July 30, 2026 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix Bug fix security Security related issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants