Skip to content

fix(cli_control): harden process kill and error paths - #155

Merged
mkoura merged 1 commit into
masterfrom
fix/cli-control-hardening
Aug 17, 2026
Merged

fix(cli_control): harden process kill and error paths#155
mkoura merged 1 commit into
masterfrom
fix/cli-control-hardening

Conversation

@mkoura

@mkoura mkoura commented Aug 17, 2026

Copy link
Copy Markdown
Collaborator
  • trust start script PID only while instance is starting; a finished start leaves a stale PID that the kernel may recycle
  • reject PID <= 0 from pidfile; 0 or negative would signal the whole process group
  • escalate SIGTERM -> SIGKILL with bounded waits instead of a blind 0.5s sleep; report a survivor as failure
  • catch OSError from Popen in start/stop/restart helpers; a script without exec bit was an unhandled traceback
  • undelay instance in finally; a raised exception used to leave the delay file behind and abort the whole stopall loop
  • check running state only while the instance is delayed (TOCTOU)
  • validate action flags and supervisorctl presence before delaying
  • tolerate malformed testnet.json in ls; warn instead of crashing
  • shlex-quote sourced env values; unquoted workdir path could break quoting or execute command substitution
  • pass argv lists to run_command; str splitting broke on workdir paths with spaces
  • remove stale pidfile also on foreground start

Copilot AI 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.

Pull request overview

This PR hardens the testnet CLI control/create flows to avoid unsafe PID signaling, improve process termination reliability, and make command execution and instance listing more robust in the presence of malformed/missing files.

Changes:

  • Adds PID validation helpers and a bounded SIGTERM→SIGKILL escalation path when stopping “starting” instances.
  • Switches command execution to argv lists (avoids .split() issues on paths with spaces) and expands error handling for OSError.
  • Quotes exported environment values for safe shell sourcing and makes ls resilient to malformed/missing testnet.json.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
src/cardonnay/cli_create.py Quotes sourced env values, uses argv lists for run_command, and adjusts PID file handling for start paths.
src/cardonnay/cli_control.py Implements PID validation + safer kill logic, hardens stop/restart execution paths, improves ls robustness, and refactors action/delay handling.
Suppressed comments (1)

src/cardonnay/cli_control.py:302

  • The earlier guard against negative instance_num was removed. With a negative value, delay_instance() will create a delay_stat-<n> file, triggering warnings in get_delay_instances() and potentially interfering with instance coordination. Reject negative instance numbers up front.
    workdir_pl = ca_utils.get_workdir(workdir=workdir).absolute()

    if not (stop or restart or restart_nodes):
        LOGGER.error("No valid action was selected.")
        return 1

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/cardonnay/cli_create.py Outdated
Comment thread src/cardonnay/cli_control.py
- trust start script PID only while instance is starting; a finished
  start leaves a stale PID that the kernel may recycle
- reject PID <= 0 from pidfile; 0 or negative would signal the whole
  process group
- escalate SIGTERM -> SIGKILL with bounded waits instead of a blind
  0.5s sleep; report a survivor as failure
- catch OSError from Popen in start/stop/restart helpers; a script
  without exec bit was an unhandled traceback
- undelay instance in finally; a raised exception used to leave the
  delay file behind and abort the whole stopall loop
- check running state only while the instance is delayed (TOCTOU)
- validate action flags and supervisorctl presence before delaying
- tolerate malformed testnet.json in ls; warn instead of crashing
- shlex-quote sourced env values; unquoted workdir path could break
  quoting or execute command substitution
- pass argv lists to run_command; str splitting broke on workdir
  paths with spaces
- remove stale pidfile also on foreground start
@mkoura
mkoura force-pushed the fix/cli-control-hardening branch from 2f2af14 to 64f23de Compare August 17, 2026 12:55
@mkoura
mkoura merged commit 550b24b into master Aug 17, 2026
4 checks passed
@mkoura
mkoura deleted the fix/cli-control-hardening branch August 17, 2026 13:03
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.

2 participants