Skip to content

fix: concore stop can kill unrelated processes (false positive pattern matching) - #622

Open
Somya-jain0811 wants to merge 1 commit into
ControlCore-Project:devfrom
Somya-jain0811:fix/stop-false-positive-process-matching
Open

Somya-jain0811 wants to merge 1 commit into
ControlCore-Project:devfrom
Somya-jain0811:fix/stop-false-positive-process-matching

Conversation

@Somya-jain0811

@Somya-jain0811 Somya-jain0811 commented Oct 29, 2026 •

Copy link
Copy Markdown

Problem

Fixes #623

concore stop used broad substring matching ('concore' in cmdline) to detect processes. This matched editors, git, IDEs, and anything else that touched a file with 'concore' in its path - potentially killing unrelated processes.

For example, running concore stop while VS Code had a concore file open (code concore/README.md) would kill the editor. Same for git -C concore pull or grep -r concore ./src.

Fix

Extracted shared detection into process_utils.py with a 5-tier strict approach:

  1. PID registry - trust concorekill_pids.txt written at study startup (most reliable)
  2. concorekill.bat - only spawned by concore itself
  3. Regex matching - only matches actual concore executions (python concore_*.py, python -m concore, concore build/stop/...)
  4. CLI token matching - standalone concore CLI commands
  5. cwd marker files - check for concore.iport + concore.py in working directory

Updated both stop.py and status.py to use the new detector.

Tests

Added 19 test cases in tests/test_process_detection.py:

  • True positives: study nodes, CLI commands, module invocations, Windows paths
  • False positives: VS Code, vim, git, grep, notepad, explorer - all correctly rejected
  • PID registry: match/miss scenarios
  • cwd markers: match/miss scenarios
  • Edge cases: empty/None cmdlines

Files Changed

  • concore_cli/commands/process_utils.py (new) - shared detection logic
  • concore_cli/commands/stop.py - uses strict detector
  • concore_cli/commands/status.py - uses strict detector
  • tests/test_process_detection.py (new) - comprehensive test suite

@github-actions

Copy link
Copy Markdown

🎉 Welcome aboard, @Somya-jain0811! Thank you for your first pull request in concore.

Please ensure that you are contributing to the dev branch.

Your contribution means a lot to us. We'll review it shortly.

Please ensure you have reviewed our:

If you have any questions, feel free to ask. Happy coding!

@Somya-jain0811
Somya-jain0811 changed the base branch from main to dev October 29, 2026 07:21
…ess detection

The old is_concore check ('concore' in cmdline) was dangerously broad —
it matched editors, git, IDEs, and anything else that happened to touch
a file with 'concore' in its path.

Changes:
- Extract shared detection logic into process_utils.py
- Use regex patterns that only match actual concore executions
  (python concore_*.py, python -m concore, concore CLI commands)
- Trust the PID registry (concorekill_pids.txt) as primary source
- Detect concorekill.bat as a concore-owned process
- Update stop.py and status.py to use the new strict detector
- Add comprehensive tests covering true positives, false positives,
  PID registry, and edge cases
@Somya-jain0811

Copy link
Copy Markdown
Author

Hey @pradeeban

Wanted to clarify how this relates to #581.

PR #581 solved the false-positive issue by checking cwd marker files, but testing revealed two cases where study processes still get missed:

  1. psutil.AccessDenied on proc.cwd(): Very common across OS permissions. When cwd() fails, it defaults to None and active concore nodes are silently left running as orphan processes.
  2. Relative launches: Running nodes from repo root (e.g., python studies/node1/concore_plant.py) means cwd lacks concore.iport, so concore stop misses them.

What this PR does:

  • Checks concorekill_pids.txt first as the source of truth (concore already writes this at startup).
  • Adds regex/token matching so real node executions are caught even if cwd() is inaccessible or launched from outside the folder.
  • Keeps Bhuvan's marker check from fix: replace raw substring match with real markers when detecting concore processes #581 as a fallback.
  • Adds 28 test scenarios covering true positives, false positives (editors/git), and permission edge cases.

Happy to clean up / alias _process_match.py into process_utils.py so there's no duplicate code. Let me know what you think!

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