Uh oh!
There was an error while loading. Please reload this page.
fix(cli): stop --web-bg reporting false success for workflows that never start - #417
Merged
Merged
Conversation
…ver start The background launcher used to trust a bare TCP connect as proof the workflow started: the moment anything accepted a connection on the dashboard port, it wrote the PID file, printed the URL, and exited 0 — even for a workflow that failed load_config moments later. Two changes close this: 1. WebDashboard.start() (which binds the port) now runs only after load_config succeeds in run_workflow_async, so a ConfigurationError from a broken workflow never binds a port in the first place. 2. _finalize_background_launch now confirms the workflow actually started, not just that a socket answered. _wait_for_server checks the child's exit status on every iteration of its connect loop, so a dead child is detected in well under a second instead of after the full 15s timeout. A new stage-two probe (_wait_for_workflow_start) then polls GET /api/info (the same identity endpoint `conductor stop` already uses) for up to 30s (CONDUCTOR_WEB_BG_START_TIMEOUT, `0` disables it) until it reports a workflow_started event, exiting 1 with the exit code and a bounded tail of the captured stderr log if the child dies first, or naming the conflicting PID if the port turns out to be held by an unrelated process. The PID file is still written as soon as the port opens — before the stage-two wait — so a slow-starting run stays visible to `conductor status`/`stop` throughout; if the child then dies, the entry is removed. Passing the stage-two deadline with the child still alive is not treated as a failure: the URL is still printed, alongside a note that the workflow hasn't reported starting yet. Fixes#410 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Code review of #417 turned up three issues where the still-in-progress false-success fix had gaps of its own: 1. _finalize_background_launch returned bare True for two cases where the child had already exited cleanly (before the port opened, or during the stage-two workflow-start wait), but app.py printed the dashboard URL and "running in background" unconditionally on that same True — a narrower recurrence of the exact bug this PR fixes. Add BackgroundLaunch.still_running, computed from a fresh proc.poll() in _spawn_bg_child, and gate the "running" output on it; print a new "workflow completed" notice instead when the child already exited. 2. run_workflow_async's (and the resume-path's) "dashboard failed to start, continue without it" fallback silently swallowed the error in --web-bg mode, which then left the port unreachable — causing _finalize_background_launch to either report false success (fast workflow) or kill an otherwise-healthy long-running workflow, in both cases hiding the real dashboard error. In bg mode, propagate the failure instead so it surfaces through the launcher's existing "process exited" error path. 3. _probe_workflow_info's blanket except Exception folded genuine defects (an HTTP error status, a bug in this function) into the same "not ready yet" bucket as connection/timeout failures, making a persistent problem indistinguishable from ordinary startup latency for the full 30s wait. Narrow the catch to httpx.HTTPError and ValueError (JSON decode failures) at debug, and log anything else at warning with the exception type. Found via a multi-agent code-review pass (code-reviewer, pr-test-analyzer, silent-failure-hunter, type-design-analyzer, comment-analyzer, dead-code-finder, code-simplifier); the three fixes above address the blocking findings from type-design-analyzer and silent-failure-hunter. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… gaps A second code-review pass on the follow-up commit turned up four issues in that commit's own fixes: 1. bg_mode (used to decide whether a dashboard-start failure should propagate as fatal) was read from CONDUCTOR_WEB_BG alone -- an ordinary env var inherited by every descendant of a --web-bg child, not just the one process the launcher tracks. A workflow that shells out and spawns a fresh, non-bg `conductor run --web` would inherit CONDUCTOR_WEB_BG=1 and be wrongly hard-failed on a dashboard error that should have fallen back gracefully. Added _is_scoped_bg_child(), which cross-checks CONDUCTOR_WEB_PORT against this invocation's own web_port, mirroring cli/self_run.py's existing scoping pattern for the same inheritance hazard. 2. The new "propagate as RuntimeError" branch raised before resetting dashboard = None, so the unconditional `finally: dashboard.stop()` could await a serve task that never came up -- re-raising the original failure (or a bare SystemExit from uvicorn, which isn't even an Exception) and silently replacing the informative RuntimeError. Now sets dashboard = None (and, on resume, engine.clear_web_dashboard()) before the bg_mode check. 3. BackgroundLaunch.still_running's docstring flatly contradicted itself: one sentence said a clean sub-second run makes it False, the next said the same scenario makes it True. Corrected to state the fields deliberately diverge, and cross-referenced the check order (still_running before workflow_started) on both fields. 4. still_running was computed via a bare proc.poll() with no exit-code check, so a child that crashed (non-zero) in the narrow window between _finalize_background_launch reporting success and the final re-poll would be reported as a clean "Workflow completed" -- a residual false-success gap. _spawn_bg_child now raises instead when it observes a non-zero exit in that window. Also added regression tests: scoped bg_mode fallback/propagation for both run and resume (including the leaked-env-var scenario), CLI-level still_running notice tests for both commands, and a race-condition test for the new still_running exit-code check. Full test suite (6078 tests) passes; ruff and ty clean. Found via a second round of parallel code review (code-reviewer, pr-test-analyzer, silent-failure-hunter, type-design-analyzer, comment-analyzer, dead-code-finder, code-simplifier) against the previous follow-up commit. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Jason Robert (jrob5756)
marked this pull request as ready for review
August 12, 2026 13:58
…success # Conflicts: # CHANGELOG.md
Uh oh!
There was an error while loading. Please reload this page.
Jason Robert (jrob5756) pushed a commit
that referenced
this pull request
Aug 14, 2026
Resolves the overlap between the Fleet Manager's run-record launch gate (D2) and the two-stage readiness contract main added in #410/#417. Both survive; they are complementary rather than alternatives: - Stage one (`_wait_for_server`) is unchanged. - The parent-side `write_pid_file` main wrote between the stages is replaced by this branch's poll for the *child's* run record. That is a strictly stronger signal -- the child only writes the record once it is executing -- and preserves D2's single-writer invariant, which a reinstated parent-side write would break. - Stage two (`_wait_for_workflow_start`, `/api/info`) is retained, so `workflow_started` / `still_running` still distinguish "listening" from "actually started" and from "already exited". - The stage-two failure cleanup moves from `remove_pid_file_at` to a new `_remove_dead_child_record`, identity-checked on `pid` for the same reason (issue #344): a resumed launch can carry a checkpoint's original `run_id`, so the record under that key may belong to a live process. Test resolution: both sides' new test classes are kept. Main's stage-two tests are re-pointed from the PID file to the run record. Nineteen with-blocks that predate stage two now skip it explicitly -- without that they ran the real 30s `/api/info` wait, taking `test_bg_runner.py` from 2s to 4m33s. Docs: AGENTS.md's `bg_runner.py` bullet describes the merged three-stage sequence; the env-var table keeps main's rewritten `CONDUCTOR_GATE_TOKEN` row (#424) and `CONDUCTOR_WEB_ALLOW_ORIGINS` alongside this branch's `CONDUCTOR_HOME` and `CONDUCTOR_FLEET_NO_ANIM`; CHANGELOG keeps both sides' entries. 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 freeto 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.
Summary
--web-bgused to trust a bare TCP connect as proof the workflow started: the moment anything accepted a connection on the dashboard port, it wrote the PID file, printed the URL, and exited 0 — even for a workflow that failedload_configmoments later.Two changes close this:
WebDashboard.start()(which binds the port) now runs only afterload_configsucceeds inrun_workflow_async, so aConfigurationErrorfrom a broken workflow never binds a port in the first place._finalize_background_launchnow confirms the workflow actually started, not just that a socket answered:_wait_for_serverchecks the child's exit status on every iteration of its connect loop, so a dead child is detected in well under a second instead of after the full 15s timeout._wait_for_workflow_start) pollsGET /api/info(the same identity endpointconductor stopalready uses) for up to 30s (CONDUCTOR_WEB_BG_START_TIMEOUT,0disables it) until it reports aworkflow_startedevent, exiting 1 with the exit code and a bounded tail of the captured stderr log if the child dies first, or naming the conflicting PID if the port turns out to be held by an unrelated process.The PID file is still written as soon as the port opens — before the stage-two wait — so a slow-starting run stays visible to
conductor status/stopthroughout; if the child then dies, the entry is removed. Passing the stage-two deadline with the child still alive is not treated as a failure: the URL is still printed, alongside a note that the workflow hasn't reported starting yet.Closes#410
Test plan
tests/test_cli/test_bg_runner.py— new coverage for_wait_for_serverexit-detection,_wait_for_workflow_start(STARTED/CHILD_EXITED/PORT_CONFLICT/TIMED_OUT),_resolve_start_timeout,_tail_log, and_finalize_background_launch's end-to-end wiring.tests/test_cli/test_resume_command.py,tests/test_cli/test_web_flags.py— updated/added coverage for the dashboard-start-after-load_config ordering andworkflow_startednotice plumbing.tests/test_config/test_instructions.py— minor adjustment for the reordered startup sequence.