fix(host): run session runners in the workspace, not the daemon's cwd (#3974)

* fix(host): run session runners in the workspace, not the daemon's cwd

A host daemon started from a directory that later disappears (a temp
checkout, a removed worktree) passes that dead cwd to every runner it spawns.
Path.cwd() then raises FileNotFoundError inside the runner and native
sessions fail with "Native Pi terminal failed to start" — hit live while
verifying the pi-native gateway fix.

Spawn the runner with cwd=<session workspace>, which _build_runner_env
already documents as the runner's cwd and which is verified to exist just
above the spawn.

Signed-off-by: David O'Keeffe <david.okeeffe@databricks.com>

* chore: retrigger CI (flaky integration test)

Signed-off-by: David O'Keeffe <david.okeeffe@databricks.com>

* fix(host): require an explicit runner workspace on the zygote fork path

fork_runner defaulted workspace to os.getcwd() — the daemon's cwd, the
exact value the workspace fix exists to avoid. The forked child was
already strict (it raises when the request carries no cwd), so the
manager was the only lenient link: a call site that omitted the argument
silently resurrected the deleted-cwd crash instead of failing loudly.

Make the parameter required so both ends agree, and cover the zygote
fork path's cwd, which had no test — only the direct Popen path did.

---------

Signed-off-by: David O'Keeffe <david.okeeffe@databricks.com>
Co-authored-by: dbczumar <corey.zumar@databricks.com>
This commit is contained in:
David O'Keeffe
2026-08-08 03:23:15 +10:00
committed by GitHub
parent 29eb8ff242
commit 3419de8da6
5 changed files with 68 additions and 19 deletions
+6 -2
View File
@@ -1258,7 +1258,7 @@ class HostProcess:
# zygote would retain its exit status forever. On cancellation we let
# the spawn land and then tear that runner down.
spawn = asyncio.ensure_future(
asyncio.to_thread(self._spawn_runner_proc, env, _session_slug)
asyncio.to_thread(self._spawn_runner_proc, env, _session_slug, workspace)
)
try:
proc, log_path = await asyncio.shield(spawn)
@@ -1312,6 +1312,7 @@ class HostProcess:
self,
env: dict[str, str],
session_slug: str,
workspace: Path,
) -> tuple[subprocess.Popen[bytes] | ZygoteRunnerProc, Path]:
"""Open the session log and spawn the runner, via zygote or direct Popen.
@@ -1325,6 +1326,7 @@ class HostProcess:
:param env: Runner environment from :func:`_build_runner_env` (its
``RUNNER_PARENT_PID`` is the daemon pid; overridden on the zygote path).
:param session_slug: Sanitized session id fragment for the log filename.
:param workspace: Existing session workspace to use as the runner's cwd.
:returns: ``(process_handle, log_path)`` — the handle quacks like Popen.
:raises OSError: If the log file or a direct Popen spawn fails.
"""
@@ -1340,7 +1342,7 @@ class HostProcess:
# getppid()-based orphan check must watch the zygote pid.
zygote_env = dict(env)
zygote_env[RUNNER_PARENT_PID_ENV_VAR] = str(zygote.pid)
proc = zygote.fork_runner(zygote_env, str(log_path))
proc = zygote.fork_runner(zygote_env, str(log_path), str(workspace))
_logger.info(
"Forked runner via zygote (zygote pid=%s, runner pid=%s)",
zygote.pid,
@@ -1364,6 +1366,8 @@ class HostProcess:
proc = subprocess.Popen(
[sys.executable, "-m", "omnigent.runner._entry"],
env=env,
# A daemon may outlive the checkout it started from.
cwd=str(workspace),
# Runners are WS-tunnel clients with no interactive input.
# Give them a clean /dev/null stdin instead of inheriting the
# daemon's: a long-lived daemon (e.g. backgrounded / nohup'd)
+13 -2
View File
@@ -235,17 +235,28 @@ class ZygoteManager:
stderr=log_fh,
)
def fork_runner(self, env: dict[str, str], log_path: str) -> ZygoteRunnerProc:
def fork_runner(self, env: dict[str, str], log_path: str, workspace: str) -> ZygoteRunnerProc:
"""Ask the zygote to fork a runner with *env*, returning its handle.
:param env: Full runner environment (the child replaces ``os.environ``
with it). Must already carry ``RUNNER_PARENT_PID`` set to the
zygote's pid so the runner's orphan watchdog stays correct.
:param log_path: Session log path the child points stdout/stderr at.
:param workspace: Existing session workspace to use as the child's cwd.
Required: a daemon may outlive the checkout it started from, so
inheriting its cwd would make every ``Path.cwd()`` in the runner
raise ``FileNotFoundError``.
:returns: A :class:`ZygoteRunnerProc` for the forked runner.
:raises ZygoteUnavailable: If the zygote is down or reports a fork error.
"""
reply = self._exchange({"cmd": "fork", "env": env, "log_path": log_path})
reply = self._exchange(
{
"cmd": "fork",
"env": env,
"log_path": log_path,
"cwd": workspace,
}
)
if "error" in reply:
raise ZygoteUnavailable(f"zygote fork failed: {reply['error']}")
pid = reply.get("pid")
+5
View File
@@ -165,6 +165,10 @@ def _run_child(request: dict[str, Any], harness_fd: int) -> None:
zygote, exported so the runner can request harness forks.
"""
_apply_child_env(request)
workspace = request.get("cwd")
if not isinstance(workspace, str):
raise ValueError("runner fork request requires a cwd")
os.chdir(workspace)
# Tell the runner its harness-fork channel fd (set after the env replace so
# it survives the clear).
os.environ[ZYGOTE_HARNESS_FD_ENV_VAR] = str(harness_fd)
@@ -201,6 +205,7 @@ def _maybe_run_test_seam() -> None:
if test_exit is not None:
sys.stdout.write(f"marker={os.environ.get('OMNIGENT_ZYGOTE_MARKER', '')}\n")
sys.stdout.write(f"tty_fd={os.environ.get(LOG_TTY_FD_ENV_VAR, '')}\n")
sys.stdout.write(f"cwd={os.getcwd()}\n")
sys.stdout.flush()
if env_truthy(os.environ.get(_ZYGOTE_TEST_CHILD_RAISE_ENV_VAR)):
raise SystemExit(int(test_exit))
+8
View File
@@ -645,6 +645,14 @@ async def test_handle_launch_spawns_subprocess(
"runner subprocess must be spawned with stdin=subprocess.DEVNULL"
)
# Runners must start in the session workspace, not the daemon's inherited
# cwd: a daemon launched from a directory that was later deleted (temp
# checkout, removed worktree) makes every Path.cwd() in the runner raise
# FileNotFoundError, and native terminals then fail to start.
assert spawned_kwargs.get("cwd") == str(workspace), (
"runner subprocess must be spawned with cwd=<session workspace>"
)
# Clean up the spawned sleep process (and its exit watcher).
_cleanup_host(host)
+36 -15
View File
@@ -120,7 +120,7 @@ def test_fork_runner_reports_pid_and_exit_code(manager: ZygoteManager, tmp_path)
:param manager: The started manager fixture.
:param tmp_path: Temp dir for the child's log.
"""
proc = manager.fork_runner(_fork_env(0), str(tmp_path / "runner.log"))
proc = manager.fork_runner(_fork_env(0), str(tmp_path / "runner.log"), str(tmp_path))
assert isinstance(proc, ZygoteRunnerProc)
assert proc.pid != manager.pid
assert _wait_exit(proc) == 0
@@ -132,7 +132,7 @@ def test_fork_runner_nonzero_exit_is_reported(manager: ZygoteManager, tmp_path)
:param manager: The started manager fixture.
:param tmp_path: Temp dir for the child's log.
"""
proc = manager.fork_runner(_fork_env(7), str(tmp_path / "runner.log"))
proc = manager.fork_runner(_fork_env(7), str(tmp_path / "runner.log"), str(tmp_path))
assert _wait_exit(proc) == 7
@@ -148,7 +148,7 @@ def test_child_systemexit_code_is_preserved(manager: ZygoteManager, tmp_path) ->
"""
env = _fork_env(5)
env["OMNIGENT_RUNNER_ZYGOTE_TEST_CHILD_RAISE"] = "1"
proc = manager.fork_runner(env, str(tmp_path / "runner.log"))
proc = manager.fork_runner(env, str(tmp_path / "runner.log"), str(tmp_path))
assert _wait_exit(proc) == 5
@@ -158,9 +158,9 @@ def test_zygote_serves_multiple_forks(manager: ZygoteManager, tmp_path) -> None:
:param manager: The started manager fixture.
:param tmp_path: Temp dir for the child logs.
"""
first = manager.fork_runner(_fork_env(0), str(tmp_path / "a.log"))
first = manager.fork_runner(_fork_env(0), str(tmp_path / "a.log"), str(tmp_path))
assert _wait_exit(first) == 0
second = manager.fork_runner(_fork_env(3), str(tmp_path / "b.log"))
second = manager.fork_runner(_fork_env(3), str(tmp_path / "b.log"), str(tmp_path))
assert second.pid != first.pid
assert _wait_exit(second) == 3
@@ -174,7 +174,7 @@ def test_signal_of_exited_runner_is_a_safe_noop(manager: ZygoteManager, tmp_path
:param manager: The started manager fixture.
:param tmp_path: Temp dir for the child's log.
"""
proc = manager.fork_runner(_fork_env(0), str(tmp_path / "runner.log"))
proc = manager.fork_runner(_fork_env(0), str(tmp_path / "runner.log"), str(tmp_path))
assert _wait_exit(proc) == 0
proc.terminate()
proc.kill()
@@ -190,7 +190,7 @@ def test_poll_after_stop_reports_live_runner_as_none(manager: ZygoteManager, tmp
:param manager: The started manager fixture.
:param tmp_path: Temp dir for the child's log.
"""
proc = manager.fork_runner(_sleep_env(30), str(tmp_path / "runner.log"))
proc = manager.fork_runner(_sleep_env(30), str(tmp_path / "runner.log"), str(tmp_path))
manager.stop()
try:
assert proc.poll() is None # pid still alive -> honestly "still live"
@@ -206,7 +206,7 @@ def test_fork_after_stop_raises_unavailable(manager: ZygoteManager, tmp_path) ->
"""
manager.stop()
with pytest.raises(ZygoteUnavailable):
manager.fork_runner(_fork_env(0), str(tmp_path / "runner.log"))
manager.fork_runner(_fork_env(0), str(tmp_path / "runner.log"), str(tmp_path))
def test_child_env_is_isolated_between_forks(manager: ZygoteManager, tmp_path) -> None:
@@ -225,9 +225,9 @@ def test_child_env_is_isolated_between_forks(manager: ZygoteManager, tmp_path) -
env_b = _fork_env(0)
env_b["OMNIGENT_ZYGOTE_MARKER"] = "bbb"
proc_a = manager.fork_runner(env_a, str(log_a))
proc_a = manager.fork_runner(env_a, str(log_a), str(tmp_path))
assert _wait_exit(proc_a) == 0
proc_b = manager.fork_runner(env_b, str(log_b))
proc_b = manager.fork_runner(env_b, str(log_b), str(tmp_path))
assert _wait_exit(proc_b) == 0
assert proc_a.pid != proc_b.pid
@@ -235,6 +235,26 @@ def test_child_env_is_isolated_between_forks(manager: ZygoteManager, tmp_path) -
assert "marker=bbb" in log_b.read_text()
def test_forked_runner_runs_in_the_requested_workspace(manager: ZygoteManager, tmp_path) -> None:
"""The forked child chdirs into the workspace, not the zygote's own cwd.
A daemon may outlive the checkout it started from, so a runner that
inherited its cwd would raise ``FileNotFoundError`` from every
``Path.cwd()``. The seam child echoes its cwd to its log.
:param manager: The started manager fixture (cwd = the pytest process's).
:param tmp_path: Temp dir for the workspace and the child's log.
"""
workspace = tmp_path / "workspace"
workspace.mkdir()
log = tmp_path / "runner.log"
proc = manager.fork_runner(_fork_env(0), str(log), str(workspace))
assert _wait_exit(proc) == 0
assert f"cwd={workspace.resolve()}\n" in log.read_text()
def test_stale_payload_tty_fd_is_cleared_in_child(manager: ZygoteManager, tmp_path) -> None:
"""A daemon-side OMNIGENT_LOG_TTY_FD in the payload never leaks as-is.
@@ -248,7 +268,7 @@ def test_stale_payload_tty_fd_is_cleared_in_child(manager: ZygoteManager, tmp_pa
env = _fork_env(0)
env["OMNIGENT_LOG_TTY_FD"] = "999" # bogus daemon-side number
log = tmp_path / "runner.log"
proc = manager.fork_runner(env, str(log))
proc = manager.fork_runner(env, str(log), str(tmp_path))
assert _wait_exit(proc) == 0
assert "tty_fd=\n" in log.read_text()
@@ -454,15 +474,16 @@ def _sleep_env(seconds: float) -> dict[str, str]:
}
def test_poll_after_zygote_crash_reports_dead_runner(manager: ZygoteManager) -> None:
def test_poll_after_zygote_crash_reports_dead_runner(manager: ZygoteManager, tmp_path) -> None:
"""A vanished zygote surfaces a dead runner as failed, never eternal alive.
Without this the daemon's ``_watch_runner`` loops forever on ``poll() is
None`` and reports a gone session as alive.
:param manager: The started manager fixture.
:param tmp_path: Temp dir used as the forked child's workspace.
"""
proc = manager.fork_runner(_sleep_env(30), "/dev/null")
proc = manager.fork_runner(_sleep_env(30), "/dev/null", str(tmp_path))
assert proc.poll() is None # child is alive
# Kill the zygote out from under the still-live runner.
@@ -503,7 +524,7 @@ def test_dropped_runner_harness_exit_codes_do_not_leak(manager: ZygoteManager, t
:param tmp_path: Temp dir for the harness child's log.
"""
# A runner that lives long enough to host a harness fork, then exits.
proc = manager.fork_runner(_sleep_env(2), "/dev/null")
proc = manager.fork_runner(_sleep_env(2), "/dev/null", str(tmp_path))
runner_pid = proc.pid
# The runner's own control socket back to the zygote is internal to the
@@ -606,7 +627,7 @@ def test_zygote_still_forks_after_a_malformed_request(manager: ZygoteManager, tm
:param tmp_path: Temp dir for the child's log.
"""
assert "error" in _raw_exchange(manager, b"[]\n")
proc = manager.fork_runner(_fork_env(0), str(tmp_path / "runner.log"))
proc = manager.fork_runner(_fork_env(0), str(tmp_path / "runner.log"), str(tmp_path))
assert _wait_exit(proc) == 0