Skip to content

Conversation

chris-sanders
Copy link
Owner

Summary

  • Replace ps -ef and pkill -f subprocess calls with psutil equivalents
  • Add psutil dependency to both pyproject.toml and .melange.yaml
  • Eliminate external command dependencies that could fail in minimal containers
  • Add comprehensive TDD tests to verify the fix

Test plan

  • All existing unit tests pass (198/202 passed, 4 skipped)
  • New ps/pkill dependency tests pass (4/4 tests)
  • Code quality checks pass (black, ruff, mypy)
  • psutil functionality verified in both development and container environments

Changes Made

  1. Added psutil dependency: Updated pyproject.toml and .melange.yaml
  2. Replaced 4 subprocess calls in bundle.py:
    • Line 1324: ps -efpsutil.process_iter() for finding processes
    • Line 1368: pkill -f "sbctl serve" → psutil process termination
    • Line 2118: ps -efpsutil.process_iter() for process checking
    • Line 2130: pkill -f "sbctl" → psutil process termination
  3. Added comprehensive tests: Created test_ps_pkill_dependency.py with TDD approach
  4. Maintained compatibility: Same error handling, logging, and logic flow

This eliminates the last major external command dependencies and makes the server more reliable in minimal container environments.

Replace ps/pkill subprocess calls at lines 1324, 1368, 2118, 2130 with
psutil-based process management to eliminate external command dependencies.

Changes:
- Add psutil import to bundle.py
- Replace ps -ef subprocess.run with psutil.process_iter() for process discovery
- Replace pkill -f subprocess.run with psutil Process.terminate() for process termination
- Add proper exception handling for psutil.NoSuchProcess, AccessDenied, ZombieProcess
- Maintain same logic flow and error handling patterns
- Add types-psutil dev dependency for mypy compatibility

All existing tests pass and new ps/pkill dependency tests verify the fix works.
…ation

- Create TDD functional test that exercises actual bundle cleanup behavior
- Test catches ANY missing cleanup dependencies, not just ps/pkill
- Simulates minimal container environment to verify external command elimination
- Test validates cleanup works without external dependencies after psutil fix
- Includes container-specific test marker for actual container validation
- Remove unused imports (patch, MagicMock, shutil)
- Fix f-strings without placeholders
- All ruff, black, and mypy checks now pass
@chris-sanders chris-sanders merged commit cbcf736 into main Jul 28, 2025
6 checks passed
@chris-sanders chris-sanders deleted the task/audit-subprocess-deps branch July 28, 2025 15:49
chris-sanders added a commit that referenced this pull request Aug 11, 2025
…43)

* Replace subprocess calls with psutil equivalents in bundle.py

Replace ps/pkill subprocess calls at lines 1324, 1368, 2118, 2130 with
psutil-based process management to eliminate external command dependencies.

Changes:
- Add psutil import to bundle.py
- Replace ps -ef subprocess.run with psutil.process_iter() for process discovery
- Replace pkill -f subprocess.run with psutil Process.terminate() for process termination
- Add proper exception handling for psutil.NoSuchProcess, AccessDenied, ZombieProcess
- Maintain same logic flow and error handling patterns
- Add types-psutil dev dependency for mypy compatibility
chris-sanders added a commit that referenced this pull request Aug 11, 2025
…43)

* Replace subprocess calls with psutil equivalents in bundle.py

Replace ps/pkill subprocess calls at lines 1324, 1368, 2118, 2130 with
psutil-based process management to eliminate external command dependencies.

Changes:
- Add psutil import to bundle.py
- Replace ps -ef subprocess.run with psutil.process_iter() for process discovery
- Replace pkill -f subprocess.run with psutil Process.terminate() for process termination
- Add proper exception handling for psutil.NoSuchProcess, AccessDenied, ZombieProcess
- Maintain same logic flow and error handling patterns
- Add types-psutil dev dependency for mypy compatibility
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.

1 participant