Uh oh!
There was an error while loading. Please reload this page.
fix(cli): stop --web-bg false-flagging port conflicts on trampoline re-exec - #445
Merged
Jason Robert (jrob5756) merged 2 commits intoAug 16, 2026
Merged
Conversation
…e-exec The launch gate's identity checks compared against the spawned process's pid (subprocess.Popen.pid), which isn't always the pid of the process that ends up running the workflow when sys.executable re-execs into a different one (e.g. a uv tool install trampoline on Windows). That made the run-record poll never see its own child's record and made the /api/info probe report every port as held by a foreign process, terminating a healthy child with a false "Port already in use". The run-record poll now also accepts a record whose pid differs from Popen.pid when the record is fresh, and carries the record's real pid forward as the confirmed identity for stage two. A PORT_CONFLICT is now only raised when that identity was confirmed; an unconfirmed mismatch degrades to the existing "still initializing" note. The foreign pid is also captured before the child is terminated instead of probed after. Closes#444
Addresses PR #444 review findings: - _classify_dashboard_identity no longer falls back to comparing run_id when the payload reports a usable int pid but this launch's own child_pid is unconfirmed. That fallback misjudged a resumed run's own healthy dashboard as FOREIGN (run_id legitimately differs on resume), suppressing a real workflow_started and stalling conductor resume --web-bg for the full 30s timeout. - Added direct unit coverage for all five _classify_dashboard_identity branches, plus a regression test pinning the deliberate `elif "started_at" in info` skip for an unconfirmed-FOREIGN payload. - Added boundary/contract tests for _record_is_fresh at the realistic near-equal-instant margin (the actual boundary a real writer produces), including a producer/consumer test using a real RunRecord round-tripped through write_run_record/read_run_record, and updated the trampoline freshness e2e test to stamp started_at at the real spawn instant instead of an artificial +1h offset. Also applied several review recommendations: - CHILD_EXITED cleanup no longer deletes a run record without confirming the owning pid is actually dead, and the previous truthy-or fallback (`confirmed_child_pid or proc.pid`) is now an explicit `is not None` check. - Renamed _run_record_matches_launch to _confirmed_pid_from_record, returning the confirmed pid (int | None) instead of a bool, removing both `# type: ignore[union-attr]` comments. - Removed an unreachable `except TypeError` arm in _record_is_fresh. - Hardened the real two-process regression test: bounded the handshake read with a timeout so a hung nested interpreter can't block the suite, and killed the reparented inner process in `finally` so it no longer leaks. - Distinguished proc.pid from the confirmed/record pid in several tests that previously used equal values for both, so they can no longer pass merely by coincidence. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Jason Robert (jrob5756)
marked this pull request as ready for review
August 16, 2026 01:21
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Aug 16, 2026
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
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
Fixes a bug where
--web-bgcould fail with a false "Port already inuse" and terminate a healthy background run.
The launch gate's two identity checks both compared against the
spawned process's pid (
subprocess.Popen.pid), which is not alwaysthe pid of the process that ends up running the workflow: on a
trampoline
sys.executable(e.g. auv tool installon Windows, thedocumented install path) the spawned process re-execs into a different
one. That made the run-record poll never see its own child's record —
surfacing as "did not report a run record within 15 seconds, but is
still running" — and then made the
/api/infoprobe report every portas held by a foreign process, terminating the healthy child.
The run-record poll now also accepts a record whose
piddiffers fromPopen.pidwhen the record is fresh, and carries the record's realpidforward as the confirmed identity for the port-conflict check. APORT_CONFLICTis now only raised when that identity was confirmed;an unconfirmed mismatch degrades to the existing non-fatal "still
initializing" note. The foreign pid is also captured before the child
is terminated instead of probed after, when it can no longer answer.
Closes#444
Testing
tests/test_cli/test_bg_runner.pyupdated/extended to cover thetrampoline re-exec identity scenario.