fix(cli): stop no longer kills the run it is executing inside - #414
Merged
Conversation
An agent smoke-testing `conductor stop` from its own workflow's `bash` tool inherited that workflow's background environment and terminated itself -- the process printed "Stopped" and was killed by what it printed. Add `conductor.cli.self_run`, which identifies the run `stop` is executing inside via three signals, tried in order: `CONDUCTOR_RUN_ID` (set on every `--web-bg` child and inherited by descendants), the legacy `CONDUCTOR_WEB_BG`/`CONDUCTOR_WEB_PORT` pair (for PID files predating the `run_id` field), and POSIX process ancestry. That run is now excluded from targeting by default: - `--all` now means "stop all *other* runs" - the no-flag auto-stop skips it - `--port <own port>` is refused (exit 1), naming the fix - a self-only `stop`/`stop --all` prints a refusal and exits 0 - `--allow-self` restores the previous behavior exactly, printing a warning whenever it actually causes the caller's own run to be signalled Process-ancestry detection is POSIX-only; Windows relies on the env-var signals alone.
…t gaps Code review of PR #414 found several genuine issues in the self-exclusion logic itself and in the test suite's ability to catch a regression of it. Crash fixes in self_run.py (all verified with real reproductions): - partition_own_run crashed on a non-string run_id (e.g. int) in a PID file, taking down `stop` for every run, not just the malformed one. Now logs a warning and treats it as absent, matching pid.py::scan_pid_files' "skip, don't raise" discipline. - _read_ppid's `except OSError` didn't catch UnicodeDecodeError, which a non-UTF-8 /proc/<pid>/status Name: line (settable by any unprivileged process via prctl(PR_SET_NAME)) can raise. This aborted the ancestry walk before it ever reached the os.getsid(0) fallback, defeating the safety net for any of up to 64 ancestor processes. Now decodes with errors="replace" since only the PPid: line is ever parsed. - describe_own_run crashed on `"workflow": null` via Path(None) -- precisely the message meant to reassure a user that their own run is protected. Now falls back to "unknown" the same way a missing key does. Test-suite hardening: - The one test combining a self-run and an other-run under --all asserted only generic substrings/call counts, and could not detect the own/others classification being fully inverted (verified by swapping them and rerunning -- the test still passed). Now uses distinct PIDs and asserts on which PID was actually signalled. - Added coverage for the no-flag path with a mixed self+other population (auto-stop-the-sole-other and list-remaining-others branches), previously fully unexercised -- arguably the most common real trigger for issue #399. - Added a unit test for partition_own_run with genuinely mixed input, and regression tests for each of the three crash fixes above. - OwnRunPartition now validates its own/others disjointness in __post_init__ (with slots=True), turning a future swap bug into an immediate, loud construction-time failure instead of a silent behavioral inversion. Doc/comment fixes: - Removed the dead `seen`-set cycle guard in own_run_pids (the pre-existing `parent in pids` check already terminates any cycle) and corrected the docstring that misattributed cycle termination to it. - Removed the redundant _clear_default_stub test fixture that duplicated the module-level no_self_run fixture without actually overriding it (different fixture names don't override in pytest). - Fixed a rule-F misattribution in describe_own_run's docstring, an ambiguous "naming the fix" phrase in CHANGELOG.md, a muddled causal explanation and a self-contradictory "restores ... exactly" sentence in docs/cli-reference.md, and matched stop()'s docstring backtick style to its sibling commands. - Documented the case-insensitive run_id comparison in the module docstring and docs/cli-reference.md. Verification: full test suite (5885 passed, 47 skipped, 1 pre-existing unrelated flake in test_event_log.py confirmed present on the unmodified branch), ruff, and ty all pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Jason Robert (jrob5756)
marked this pull request as ready for review
August 11, 2026 20:29
Reconciles PR #414's self-exclusion feature (--allow-self, issue #399) with PR #383's escalating stop ladder (--force/--json, issue #344), both of which modified conductor stop's targeting logic on overlapping lines. - src/conductor/cli/app.py: stop() now partitions running PID-file entries into self/others (via self_run.partition_own_run) before applying the ladder-based _stop_process/--force/--json machinery, so self-exclusion and confirmed termination compose instead of one clobbering the other. - tests/test_cli/test_stop.py: rewrote TestStopSelfExclusion to drive the real ladder via _stops_cleanly() plus a wrapping spy on _stop_process (rather than asserting on direct os.kill calls, which no longer reflects how stop() signals a process), and updated the ambiguous-listing self-exclusion test to expect exit code 1, matching #383's fix for that case. - tests/test_cli/test_stop_ladder.py: added the same no_self_run autouse fixture used in test_stop.py -- these pre-existing ladder tests use small arbitrary PIDs (1, 2, 4242) that can collide with real process ancestry in shallow-PID-namespace environments, which self-exclusion would otherwise misclassify as the caller's own run. - CHANGELOG.md, docs/cli-reference.md: concatenated both PRs' entries; merged the duplicated 'Exit Codes' section in the stop docs into one table covering both self-exclusion and ladder outcomes. Verified: ruff check, ruff format --check, ty check src, and the full test suite (targeted stop/self_run/pid/markup suites plus pytest -m "not install_scripts") all pass. The one failure seen (test_event_log.py::test_filenames_unique_for_simultaneous_starts) is a pre-existing, environment-specific flake reproduced identically on a clean origin/main checkout, unrelated to this merge. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
main advanced again after the first merge, adding PR #413 (fold the conductor status/stop dashboard-URL column instead of cropping it, render Started at minute precision). Only tests/test_cli/test_stop.py conflicted, on the same TestStopAutoDetect class our self-exclusion merge touched. - Added the os/re imports #413 needs alongside our existing imports. - Kept #413's new test_lists_started_at_minute_precision case. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Jason Robert (jrob5756)
pushed a commit
that referenced
this pull request
Aug 11, 2026
Resolves a CHANGELOG.md ordering conflict with #414/#413/#388, which merged into main while this PR's review was in progress. No other files conflicted; workflow.py's auto-merge is clean (this branch's _context_window_anomaly_warned latch and main's new _pricing_hook_silent_warned latch are independent additions to the same __init__). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #399
Problem
An agent smoke-testing
conductor stopfrom its own workflow'sbashtool inherited that workflow's background environment and pointed
stopat the very run driving it — the process printed "Stopped" andwas killed by what it printed.
Fix
conductor.cli.self_run, which identifies the runstopisexecuting inside via three signals, tried in order:
CONDUCTOR_RUN_ID(set on every--web-bgchild and inherited bydescendants), the legacy
CONDUCTOR_WEB_BG/CONDUCTOR_WEB_PORTpair(for PID files predating the
run_idfield), and POSIX processancestry (
/proc/<pid>/statusPPid:walk +os.getsid(0)).conductor stopnow excludes that run from targeting by default:--allmeans "stop all other runs"--port <own port>is refused (exit 1), naming the fixstop/stop --allprints a refusal and exits0--allow-selfrestores the previous behavior exactly, printing awarning whenever it actually causes the caller's own run to be
signalled
env-var signals alone (documented caveat).
docs/cli-reference.md(Self-Exclusion + Exit Codessections),
CHANGELOG.md, andAGENTS.mdto match.Testing
uv run pytest tests/test_cli/test_stop.py tests/test_cli/test_self_run.py tests/test_cli/test_markup_injection.py— 76 passed.tests/test_cli/test_self_run.pycoversown_run_pids()identity signals, the
/procancestry walk (cycle/cap handling),partition_own_runclassification, anddescribe_own_runformatting.
tests/test_cli/test_stop.pygains aTestStopSelfExclusionclasscovering
--all, no-flag,--port, and--allow-selfbehavior;existing tests stub
own_run_pidsto keep prior targeting testsdeterministic.