From 5940144c2857fbc3ef888cf569e9cd952887398e Mon Sep 17 00:00:00 2001 From: Leo Li Date: Fri, 25 Sep 2026 14:43:50 -0400 Subject: [PATCH 1/3] glaeda-cmux-runner-hook: take-root --switch and take-gui Two step-time helpers for cmux's E2E build, behind the existing glaeda-canonical-root shim: - take ROOT --switch: lets go of the roots the job holds, then waits for ROOT, so a build that finds a product to reuse at another canonical root can move there without ever holding two roots. Only roots with a holder of their own can be let go: those from an earlier take, and the admission root of a ROOT_SWITCHERS class (compile-gui), which admission now hands to a separate holder (-root-k.pid) instead of the main one. - take-gui [--wait S]: holds the gui token from that step to the end of the job (a -gui.pid holder, released by job-completed). A no-op when admission already gave the job gui. A job in take-gui holds a root while a gui job waiting in take-root holds gui, the opposite order, so take-root waiters leave capacity/root-k.want- markers and take-gui exits 3 (gave way) at once when a gui holder waits for one of its roots. compile-gui still takes gui at admission; dropping it waits for cmux's build to call take-gui before its tests. The shim needs a fleet re-apply. Co-Authored-By: Claude Opus 5.5 --- docs/CMUX_MINI_RUNNER.md | 15 +++ scripts/glaeda-cmux-runner | 20 ++-- scripts/glaeda-cmux-runner-hook | 150 +++++++++++++++++++++++++---- scripts/test-glaeda-cmux-runner.py | 114 +++++++++++++++++++++- 4 files changed, 269 insertions(+), 30 deletions(-) diff --git a/docs/CMUX_MINI_RUNNER.md b/docs/CMUX_MINI_RUNNER.md index b631b114c..03fbb74e6 100644 --- a/docs/CMUX_MINI_RUNNER.md +++ b/docs/CMUX_MINI_RUNNER.md @@ -425,6 +425,21 @@ in at the producer's root. So one root job per root per mini: detached holder tied to the job's Runner.Worker and released by job-completed. A re-take of a root the same job already holds (a compile restoring its own product) is a no-op. It exits 1 when the root is still busy after the wait, and 2 for a bad root or outside a runner job. +- A second, different root is refused (exit 2): two jobs taking two roots in opposite orders would deadlock. + `take ROOT --switch` swaps instead. It first lets go of every root the job holds, then waits for ROOT, so + the job never holds two. It only works when each held root has a holder of its own: one taken with + `take`, or the admission root of a `ROOT_SWITCHERS` class (compile-gui, test-e2e's `build`), whose root + the hook hands to a separate holder at admission. A build that finds a product to reuse at another root + switches to it. The caller must be done with the old root. +- `glaeda-canonical-root take-gui [--wait S]` holds the mini's gui token from that step to the end of the + job, with a third holder (`-gui.pid`) that job-completed releases. It is a no-op when the job + already holds gui from admission. It exits 0 when held, 1 when still taken after the wait, and 2 + outside a job. + - A job in take-gui holds a root, while a gui job waiting in take-root holds gui: opposite lock orders. + So every take-root waiter writes `capacity/root-k.want-`, containing `gui` when its job holds + the gui token. take-gui exits 3 at once when a gui holder is waiting for a root this job holds. The + caller then leaves its console-session work to another job and finishes, which frees the root. + Stale markers (dead pids) are removed on sight. - Jobs the hook does not know (seed-swiftpm-manifests, anything new) are pinned to root 1, because they use /private/tmp/cmux-ci themselves. Ids that other workflows reuse (`build`, `test`, `lint`) are classed by (workflow file, job id) from `GITHUB_WORKFLOW_REF`, so with `canonicalRoots` above 1 test-e2e's jobs are diff --git a/scripts/glaeda-cmux-runner b/scripts/glaeda-cmux-runner index 2061a39e3..91ac41c8c 100755 --- a/scripts/glaeda-cmux-runner +++ b/scripts/glaeda-cmux-runner @@ -470,15 +470,21 @@ def hook_wrappers(ctx: Context) -> dict[str, bytes]: capacity = shlex.quote(os.fspath(Path(os.environ.get("GLAEDA_FLEET_DIR") or "/Users/Shared/cmux-build-fleet") / "capacity")) home = ctx.home if isinstance(getattr(ctx, "home", None), Path) else Path.home() state = shlex.quote(os.fspath(home / ".local/state/glaeda/cmux-runner")) + dirs = f" --capacity-dir {capacity} --state-dir {state}" root = ( "#!/bin/sh\n" - "# glaeda-canonical-root take ROOT [--wait SECONDS] (generated, receipt-owned): hold canonical root ROOT\n" - "# (/private/tmp/cmux-ci[-N]) for the rest of this job; the job-started hook links it into the fleet bin.\n" - "[ \"${1:-}\" = take ] && [ -n \"${2:-}\" ] || " - "{ echo 'usage: glaeda-canonical-root take ROOT [--wait SECONDS]' >&2; exit 2; }\n" - "root=$2; shift 2\n" - f"exec {python} {hook} take-root --root \"$root\" --canonical-roots {roots_n}" - f" --capacity-dir {capacity} --state-dir {state} \"$@\"\n" + "# glaeda-canonical-root take ROOT [--wait SECONDS] [--switch] (generated, receipt-owned): hold canonical\n" + "# root ROOT (/private/tmp/cmux-ci[-N]) for the rest of this job; take-gui [--wait SECONDS] holds the\n" + "# mini's gui token (exit 3: gave way to a gui job waiting for this job's root). The job-started hook links\n" + "# it into the fleet bin.\n" + "usage() { echo 'usage: glaeda-canonical-root take ROOT [--wait SECONDS] [--switch]" + " | take-gui [--wait SECONDS]' >&2; exit 2; }\n" + "case \"${1:-}\" in\n" + "take) [ -n \"${2:-}\" ] || usage; root=$2; shift 2\n" + f" exec {python} {hook} take-root --root \"$root\" --canonical-roots {roots_n}{dirs} \"$@\" ;;\n" + f"take-gui) shift; exec {python} {hook} take-gui{dirs} \"$@\" ;;\n" + "*) usage ;;\n" + "esac\n" ) return {"job-started.sh": started.encode(), "job-completed.sh": completed.encode(), "listen.sh": listen.encode(), "glaeda-canonical-root": root.encode()} diff --git a/scripts/glaeda-cmux-runner-hook b/scripts/glaeda-cmux-runner-hook index 752c55983..ae5ee834d 100755 --- a/scripts/glaeda-cmux-runner-hook +++ b/scripts/glaeda-cmux-runner-hook @@ -192,7 +192,16 @@ CANONICAL_ROOT_ENV = "CMUX_CI_CANONICAL_ROOT" # With more than one root, these restore a producer's product into the producer's root (#filePath is baked # in there), so they take that root from their restore step, not a token-chosen one at job start. ROOT_CONSUMERS = ("gui", "product") +# Producers whose admission root sits in a holder of its own (root_holder_file), so a step can swap it for the +# root of a product the job reuses instead of compiling (glaeda-canonical-root take ROOT --switch): the old +# root is let go before the new one is waited for, so the job never holds two roots. +ROOT_SWITCHERS = ("compile-gui",) TAKE_ROOT_POLL_S = 1.0 +# glaeda-canonical-root take-gui: a job takes the gui token only for the steps that use the console session. +# It holds a root while it waits, and a gui job (gui at admission) may be waiting in take-root for that root: +# the lock orders are opposite. So a take-root waiter leaves a marker, capacity/root-k.want- ("gui" when +# its job holds the gui token), and take-gui gives way (TAKE_GUI_GAVE_WAY) when a gui holder wants its root. +TAKE_GUI_GAVE_WAY = 3 def canonical_root(k: int) -> str: @@ -634,10 +643,13 @@ def ensure_root_shim(bin_dir: Path) -> None: os.replace(tmp, link) -def take_root(spec: str, wait_s: float, capacity_dir: Path, state_dir: Path, roots: int = 99) -> int: +def take_root(spec: str, wait_s: float, capacity_dir: Path, state_dir: Path, roots: int = 99, + switch: bool = False) -> int: """glaeda-canonical-root take ROOT: hold canonical root ROOT for the rest of this job (cmux#14338), waiting up to wait_s for it. A root this job already holds is a no-op; a second, different root is refused (two - jobs taking two roots in opposite orders would deadlock). Prints the root's path.""" + jobs taking two roots in opposite orders would deadlock), unless `switch` and every root the job holds + has a holder of its own (ROOT_SWITCHERS, or an earlier take-root): those are let go first, so the job + never holds two. The caller must be done with the old root. Prints the root's path.""" k = root_number(spec, roots) if k is None: print(f"{PREFIX}: take-root: {spec!r} is not one of this mini's {roots} canonical root(s) " @@ -653,22 +665,37 @@ def take_root(spec: str, wait_s: float, capacity_dir: Path, state_dir: Path, roo if name in held: print(path) return 0 - if held: - print(f"{PREFIX}: take-root: this job already holds {', '.join(held)}; one root per job", file=sys.stderr) + if held and not (switch and all(root_holder_file(state_dir, old).exists() for old in held)): + print(f"{PREFIX}: take-root: this job already holds {', '.join(held)}; one root per job" + + ("" if switch else " (--switch lets go of a root with its own holder first)"), file=sys.stderr) return 2 worker = runner_worker_pid() if worker is None: print(f"{PREFIX}: take-root: not inside a runner job (no Runner.Worker above this step)", file=sys.stderr) return 2 + if held: + for old in held: + release_holder(root_holder_file(state_dir, old)) + with contextlib.suppress(OSError): + roots_file(state_dir).unlink() + print(f"{PREFIX}: take-root: let go of {', '.join(held)}", file=sys.stderr) deadline = time.monotonic() + max(0.0, wait_s) - while True: - fd = lock_file(capacity_dir / f"{name}.token", fcntl.LOCK_EX) - if fd is not None: - break - if time.monotonic() >= deadline: - print(f"{PREFIX}: take-root: {path} is still in use after {wait_s:g} s", file=sys.stderr) - return 1 - time.sleep(TAKE_ROOT_POLL_S) + want = capacity_dir / f"{name}.want-{os.getpid()}" + try: + while True: + fd = lock_file(capacity_dir / f"{name}.token", fcntl.LOCK_EX) + if fd is not None: + break + if time.monotonic() >= deadline: + print(f"{PREFIX}: take-root: {path} is still in use after {wait_s:g} s", file=sys.stderr) + return 1 + if not want.exists(): # take-gui reads it: a gui holder waiting for a root must not be waited on + with contextlib.suppress(OSError): + want.write_text("gui\n" if gui_file(state_dir).exists() else "\n") + time.sleep(TAKE_ROOT_POLL_S) + finally: + with contextlib.suppress(OSError): + want.unlink() held, note = hold([fd], worker, state_dir, pid_file=root_holder_file(state_dir, name)) if not held: print(f"{PREFIX}: take-root: {note}", file=sys.stderr) @@ -679,6 +706,63 @@ def take_root(spec: str, wait_s: float, capacity_dir: Path, state_dir: Path, roo return 0 +def gui_holder_wants(capacity_dir: Path, roots: list[str]) -> str | None: + """A root in `roots` that a job holding the gui token is waiting for in take-root, from its marker.""" + for name in roots: + for marker in capacity_dir.glob(f"{name}.want-*"): + try: + pid, mark = int(marker.name.rsplit("-", 1)[1]), marker.read_text() + except (OSError, ValueError): + continue + if not pid_alive(pid): + with contextlib.suppress(OSError): + marker.unlink() # a waiter that was killed + elif mark.strip() == "gui": + return name + return None + + +def take_gui(wait_s: float, capacity_dir: Path, state_dir: Path) -> int: + """glaeda-canonical-root take-gui: hold this mini's gui token (one console session) for the rest of this + job, waiting up to wait_s. A job that already holds it (from admission) is a no-op. Exits 0 when held, 1 + when still taken after the wait, TAKE_GUI_GAVE_WAY when the holder waits for a root this job holds (see + TAKE_GUI_GAVE_WAY: the caller then leaves its console-session work to another job), 2 outside a job.""" + if not os.environ.get("RUNNER_NAME"): + print(f"{PREFIX}: take-gui: RUNNER_NAME is not set (not a runner job step)", file=sys.stderr) + return 2 + if gui_file(state_dir).exists(): + return 0 + worker = runner_worker_pid() + if worker is None: + print(f"{PREFIX}: take-gui: not inside a runner job (no Runner.Worker above this step)", file=sys.stderr) + return 2 + deadline = time.monotonic() + max(0.0, wait_s) + while True: + fd = lock_file(capacity_dir / "gui.token", fcntl.LOCK_EX) + if fd is not None: + break + held: list[str] = [] + with contextlib.suppress(OSError): + held = roots_file(state_dir).read_text().split() + wanted = gui_holder_wants(capacity_dir, held) + if wanted: + print(f"{PREFIX}: take-gui: the gui token's holder is waiting for {wanted}, which this job holds; " + "giving way", file=sys.stderr) + return TAKE_GUI_GAVE_WAY + if time.monotonic() >= deadline: + print(f"{PREFIX}: take-gui: the gui token is still taken after {wait_s:g} s", file=sys.stderr) + return 1 + time.sleep(TAKE_ROOT_POLL_S) + main = holder_file(state_dir) + ok, note = hold([fd], worker, state_dir, pid_file=main.with_name(f"{main.stem}-gui.pid")) + if not ok: + print(f"{PREFIX}: take-gui: {note}", file=sys.stderr) + return 1 + with contextlib.suppress(OSError): + gui_file(state_dir).write_text("gui\n") + return 0 + + def pid_alive(pid: int) -> bool: try: os.kill(pid, 0) @@ -812,6 +896,7 @@ def take_capacity(host_lock: str, capacity_dir: Path, total: int, job: str, watc if waiting: # flock has no writer preference: stop admitting so the build worker gets in return False, f"capacity: a fleet build is waiting for the host (pid {waiting[0]})" taken: list[str] = [] + named: dict[str, int] = {} for token in tokens: # persistent-dd comes in compile_slots copies once compile admission keeps its state per # runner; copy 0 keeps the original file name. A mini whose runners disagree on the count runs @@ -832,6 +917,7 @@ def take_capacity(host_lock: str, capacity_dir: Path, total: int, job: str, watc if fd is not None: fds.append(fd) taken.append(name) + named[name] = fd break else: what = "canonical root" if token == "root" else token @@ -847,10 +933,22 @@ def take_capacity(host_lock: str, capacity_dir: Path, total: int, job: str, watc got += 1 if got < units: return False, (f"capacity: {got} of {total} units free, {job or 'an unknown job'} ({klass}) needs {units}") - held, note = hold(fds, watch_pid, state_dir, drop=(admission,)) - fds = [] # the holder has them now, or hold closed them - what = "+".join([f"{units}/{total} units", *taken]) root = next((name for name in taken if name.startswith("root-")), None) + split = named[root] if root and klass in ROOT_SWITCHERS else None + if split is not None: # its own holder (ROOT_SWITCHERS); the main holder must not inherit it + fds.remove(split) + held, note = hold(fds, watch_pid, state_dir, drop=(admission,) if split is None else (admission, split)) + fds = [] if split is None else [split] # the holder has them now, or hold closed them + if held and split is not None: + held, why = hold(fds, watch_pid, state_dir, drop=(admission,), pid_file=root_holder_file(state_dir, root)) + fds = [] + if not held: + release_holder(holder_file(state_dir)) + note = why + what = "+".join([f"{units}/{total} units", *taken]) + if held and "gui" in taken: + with contextlib.suppress(OSError): + gui_file(state_dir).write_text("gui\n") if held and root: note += f"; {export_root(root)}" with contextlib.suppress(OSError): @@ -932,6 +1030,12 @@ def roots_file(state_dir: Path) -> Path: return main.with_name(main.stem + ".roots") +def gui_file(state_dir: Path) -> Path: + """Present while this runner's job holds the gui token (from admission or take-gui).""" + main = holder_file(state_dir) + return main.with_name(main.stem + ".gui") + + def root_holder_file(state_dir: Path, name: str) -> Path: main = holder_file(state_dir) return main.with_name(f"{main.stem}-{name}.pid") @@ -941,10 +1045,13 @@ def release_host_lock(state_dir: Path) -> str: """job-completed: tell this job's holders to let go (the admission holder and any take-root holders), and wait briefly for them.""" roots_file(state_dir).unlink(missing_ok=True) + gui_file(state_dir).unlink(missing_ok=True) main = holder_file(state_dir) extra = [release_holder(path) for path in sorted(state_dir.glob(f"{main.stem}-root-*.pid"))] + gui = [release_holder(path) for path in state_dir.glob(f"{main.stem}-gui.pid")] note = release_holder(main) - return note + (f"; {len(extra)} canonical root holder(s) released" if extra else "") + return (note + (f"; {len(extra)} canonical root holder(s) released" if extra else "") + + ("; the gui token holder released" if gui else "")) def release_holder(pid_file: Path) -> str: @@ -1825,12 +1932,14 @@ def disk_pressure(explicit: str | None) -> str: def main(argv: list[str] | None = None) -> int: p = argparse.ArgumentParser(prog=PREFIX) - p.add_argument("phase", choices=("job-started", "job-completed", "check", "listen", "take-root"), + p.add_argument("phase", choices=("job-started", "job-completed", "check", "listen", "take-root", "take-gui"), help="check: the eligibility gate only, read-only (no lock, no rustup change); " "listen: run the runner's run.sh under the listener gate (the LaunchAgent's program)") p.add_argument("--runner-dir", type=Path, help="listen: the runner directory whose run.sh the gate runs") p.add_argument("--root", help="take-root: the canonical root to hold, /private/tmp/cmux-ci[-N] or N") - p.add_argument("--wait", type=float, default=0, help="take-root: seconds to wait for the root") + p.add_argument("--wait", type=float, default=0, help="take-root, take-gui: seconds to wait for the token") + p.add_argument("--switch", action="store_true", + help="take-root: let go of the root this job holds (its own holder) and take ROOT instead") p.add_argument("--no-waiters", action="store_true", help="listen: only an exclusive holder or a reservation stops the listener (one runner per mini)") p.add_argument("--adopt", type=int, default=0, help=argparse.SUPPRESS) @@ -1886,7 +1995,9 @@ def main(argv: list[str] | None = None) -> int: if args.phase == "take-root": if not args.root: p.error("take-root needs --root") - return take_root(args.root, args.wait, args.capacity_dir, args.state_dir, args.canonical_roots) + return take_root(args.root, args.wait, args.capacity_dir, args.state_dir, args.canonical_roots, args.switch) + if args.phase == "take-gui": + return take_gui(args.wait, args.capacity_dir, args.state_dir) if args.phase == "check": why, want = node_refusal(args.fleet_class) if not why: @@ -1918,6 +2029,7 @@ def main(argv: list[str] | None = None) -> int: # The wrapper execs this script, so the parent is the job's Runner.Worker. if args.capacity_units > 0: roots_file(args.state_dir).unlink(missing_ok=True) # a crashed job's record must not look held + gui_file(args.state_dir).unlink(missing_ok=True) ensure_root_shim(Path(FLEET_DIR) / "bin") # the repository decide() admitted: the runner's variable, else the event payload's repo = os.environ.get("GITHUB_REPOSITORY") or _repo_name((event or {}).get("repository") diff --git a/scripts/test-glaeda-cmux-runner.py b/scripts/test-glaeda-cmux-runner.py index 528337ceb..ce0c67406 100644 --- a/scripts/test-glaeda-cmux-runner.py +++ b/scripts/test-glaeda-cmux-runner.py @@ -527,7 +527,13 @@ def test_capacity_admits_by_weight_and_refuses_fast_when_full(self) -> None: self.assertTrue(self.lock_free(), "every holder let go") def take(self, root: str, runner: str, *extra: str) -> subprocess.CompletedProcess: - """Run take-root as a restore step would: under a process named Runner.Worker (its ancestor). + return self.step(["take-root", "--root", root, "--canonical-roots", "2", *extra], runner) + + def take_gui(self, runner: str, *extra: str) -> subprocess.CompletedProcess: + return self.step(["take-gui", *extra], runner) + + def step(self, argv: list[str], runner: str) -> subprocess.CompletedProcess: + """Run a hook phase as a job step would: under a process named Runner.Worker (its ancestor). A real Runner.Worker outlives the step and the root holder watches it, so the stand-in reports the step's exit status and then stays up until the test ends; one that exited with the step would let the holder release the root within HOLDER_POLL_S.""" @@ -547,10 +553,9 @@ def take(self, root: str, runner: str, *extra: str) -> subprocess.CompletedProce if cc is None or subprocess.run([cc, "-o", os.fspath(worker), os.fspath(source)], capture_output=True).returncode != 0: self.skipTest("no C compiler to build a Runner.Worker stand-in") - cmd = " ".join(shlex.quote(a) for a in [sys.executable, os.fspath(HOOK), "take-root", "--root", root, + cmd = " ".join(shlex.quote(a) for a in [sys.executable, os.fspath(HOOK), *argv, "--capacity-dir", os.fspath(self.dir / "capacity"), - "--state-dir", os.fspath(self.dir / "state"), - "--canonical-roots", "2", *extra]) + "--state-dir", os.fspath(self.dir / "state")]) environ = {"PATH": "/usr/bin:/bin", "HOME": os.fspath(self.dir), "GLAEDA_RUNNER_TELEMETRY": "0", "RUNNER_NAME": runner, "GLAEDA_FLEET_DIR": os.fspath(self.dir / "fleet")} proc = subprocess.Popen([os.fspath(worker), "-c", cmd], env=environ, stdin=subprocess.PIPE, @@ -623,6 +628,103 @@ def test_two_roots_unknown_jobs_are_pinned_to_root_one(self) -> None: for runner in ("u0", "u1", "u2"): self.finish(runner) + def test_switch_swaps_a_producers_root_without_holding_two(self) -> None: + # test-e2e's build reuses a product compiled at another root: it lets its own root go, then takes that one + self.fleet() + e2e = {"GITHUB_WORKFLOW_REF": "manaflow-ai/cmux/.github/workflows/test-e2e.yml@refs/heads/main"} + two = ("--canonical-roots", "2", "--compile-slots", "2") + state = self.dir / "state" + try: + build = self.job("build", "e0", 8, None, *two, env=e2e) + self.assertIn("+gui+root-1 for build (compile-gui", build.stdout) + self.assertTrue((state / "host-lock-holder-e0-root-1.pid").exists(), "the root has a holder of its own") + refused = self.take("2", "e0", "--wait", "0") + self.assertEqual(refused.returncode, 2, "without --switch a second root is still refused") + self.assertIn("--switch", refused.stderr) + switched = self.take("/private/tmp/cmux-ci-2", "e0", "--switch") + self.assertEqual((switched.returncode, switched.stdout.strip()), (0, "/private/tmp/cmux-ci-2"), + switched.stderr) + self.assertIn("let go of root-1", switched.stderr) + self.assertEqual((state / "host-lock-holder-e0.roots").read_text().split(), ["root-2"]) + self.assertEqual(self.take("1", "c0", "--wait", "5").returncode, 0, "root 1 is free again") + gui = self.job("app-host-unit-tests", "g0", 8, None, *two, "--gui-wait", "0") + self.assertIn("the gui token is taken", gui.stdout, "the admission holder keeps units and gui") + self.assertEqual(self.take("2", "e0").returncode, 0, "a re-take of the new root is a no-op") + self.assertIn("canonical root holder(s) released", self.finish("e0")) + self.assertEqual(self.take("2", "c1", "--wait", "5").returncode, 0, "released with the job") + self.finish("c0") + self.finish("c1") + compile_ = self.job("macos-compile-admission", "p0", 8, None, *two) + self.assertIn("persistent-dd+root-1", compile_.stdout) + self.assertEqual(self.take("2", "p0", "--switch").returncode, 2, + "a root in the admission holder cannot be let go") + finally: + for runner in ("e0", "c0", "c1", "g0", "p0"): + self.finish(runner) + self.assertFalse(list((self.dir / "capacity").glob("*.want-*")), "no waiter marker outlives its wait") + self.assertTrue(self.lock_free()) + + def test_take_gui_holds_the_token_for_the_rest_of_the_job(self) -> None: + self.fleet() + two = ("--canonical-roots", "2", "--compile-slots", "2") + state = self.dir / "state" + try: + self.assertEqual(self.job("app-host-unit-tests", "g0", 8, None, *two).returncode, 0) + self.assertTrue((state / "host-lock-holder-g0.gui").exists()) + self.assertEqual(self.take_gui("g0").returncode, 0, "a job holding gui from admission: a no-op") + self.assertEqual(self.job("macos-compile-admission", "c0", 8, None, *two).returncode, 0) + waited = time.monotonic() + busy = self.take_gui("c0", "--wait", "2") + self.assertEqual(busy.returncode, 1, busy.stderr) + self.assertIn("still taken after 2 s", busy.stderr) + self.assertGreaterEqual(time.monotonic() - waited, 2) + self.finish("g0") + self.assertEqual(self.take_gui("c0", "--wait", "5").returncode, 0) + self.assertTrue((state / "host-lock-holder-c0-gui.pid").exists()) + refused = self.job("tests-build-and-lag", "g1", 8, None, *two, "--gui-wait", "0") + self.assertIn("the gui token is taken", refused.stdout) + self.assertIn("the gui token holder released", self.finish("c0")) + self.assertFalse((state / "host-lock-holder-c0.gui").exists()) + self.assertIn("+gui", self.job("tests-build-and-lag", "g2", 8, None, *two).stdout, "released with the job") + outside = self.run_hook("take-gui", None, None, "--capacity-dir", os.fspath(self.dir / "capacity"), + "--state-dir", os.fspath(state)) + self.assertEqual(outside.returncode, 2) + finally: + for runner in ("g0", "c0", "g1", "g2"): + self.finish(runner) + self.assertTrue(self.lock_free()) + + def test_take_gui_gives_way_to_a_gui_job_waiting_for_its_root(self) -> None: + # the opposite lock orders: a build holds root 1 and wants gui; a gui job holds gui and wants root 1 + self.fleet() + two = ("--canonical-roots", "2", "--compile-slots", "2") + results: dict[str, subprocess.CompletedProcess] = {} + try: + self.assertIn("root-1", self.job("macos-compile-admission", "c0", 8, None, *two).stdout) + self.assertEqual(self.job("app-host-unit-tests", "g0", 8, None, *two).returncode, 0) + waiter = threading.Thread(target=lambda: results.setdefault("g0", self.take("1", "g0", "--wait", "40"))) + waiter.start() + deadline = time.monotonic() + 15 + while not list((self.dir / "capacity").glob("root-1.want-*")) and time.monotonic() < deadline: + time.sleep(0.2) + self.assertEqual([m.read_text() for m in (self.dir / "capacity").glob("root-1.want-*")], ["gui\n"]) + waited = time.monotonic() + gave = self.take_gui("c0", "--wait", "30") + self.assertEqual(gave.returncode, hook.TAKE_GUI_GAVE_WAY, gave.stderr) + self.assertIn("waiting for root-1, which this job holds; giving way", gave.stderr) + self.assertLess(time.monotonic() - waited, 15, "it gives way at once, not after its wait") + self.finish("c0") + waiter.join(60) + self.assertEqual(results["g0"].returncode, 0, results["g0"].stderr) + # a gui holder not waiting for this job's root is no cycle: take-gui keeps waiting for the token + self.assertIn("root-2", self.job("macos-compile-admission", "c1", 8, None, *two).stdout) + self.assertEqual(self.take_gui("c1", "--wait", "1").returncode, 1) + finally: + for runner in ("c0", "g0", "c1"): + self.finish(runner) + self.assertFalse(list((self.dir / "capacity").glob("*.want-*"))) + self.assertTrue(self.lock_free()) + def test_take_root_refuses_bad_roots_and_non_jobs(self) -> None: for bad in ("/tmp/elsewhere", "/private/tmp/cmux-ci-1", "0", "cmux-ci-2x", "02", "/private/tmp/cmux-ci-02"): with self.subTest(bad=bad): @@ -2498,6 +2600,10 @@ def test_manifest_disk_floor_is_baked_into_the_hook(self) -> None: self.assertTrue((hooks / "glaeda_reservation.py").is_file()) shim = (hooks / "glaeda-canonical-root").read_text() self.assertIn("take-root --root", shim) + self.assertIn("take-gui --capacity-dir", shim) + for bad in ([], ["take"], ["nope"]): + self.assertEqual(subprocess.run(["/bin/sh", os.fspath(hooks / "glaeda-canonical-root"), *bad], + capture_output=True).returncode, 2, bad) self.assertIn("--canonical-roots 1 --capacity-dir", shim) self.assertIn("--state-dir", shim) self.assertTrue(os.access(hooks / "glaeda-canonical-root", os.X_OK)) From 95bc44564d90dd17fea0e491dd75d5ddfb1ab1bd Mon Sep 17 00:00:00 2001 From: Leo Li Date: Fri, 25 Sep 2026 14:54:07 -0400 Subject: [PATCH 2/3] glaeda-cmux-runner-hook: a switch keeps its old root until the new one is held Review follow-ups for take-root --switch and take-gui: - --switch waits for the new root while still holding the old one and lets go only once the new one is held, then writes the new CMUX_CI_CANONICAL_ROOT to $GITHUB_ENV. A timeout used to leave the job holding no root while its environment still named the old one. - A switch needs a live holder for every held root, not just its pid file: a SIGKILLed holder's leftover file no longer lets a job hold two roots. - Waiters rewrite their want marker every poll and take-gui ignores and sweeps markers older than 10 s, so a killed waiter whose pid is recycled (or belongs to another user) cannot make take-gui give way. - take-gui releases the token and exits 1 when it cannot record holding it. Co-Authored-By: Claude Opus 5.5 --- docs/CMUX_MINI_RUNNER.md | 18 ++++++---- scripts/glaeda-cmux-runner-hook | 58 ++++++++++++++++++++---------- scripts/test-glaeda-cmux-runner.py | 29 +++++++++++---- 3 files changed, 75 insertions(+), 30 deletions(-) diff --git a/docs/CMUX_MINI_RUNNER.md b/docs/CMUX_MINI_RUNNER.md index 03fbb74e6..03a3cb2c1 100644 --- a/docs/CMUX_MINI_RUNNER.md +++ b/docs/CMUX_MINI_RUNNER.md @@ -426,11 +426,16 @@ in at the producer's root. So one root job per root per mini: same job already holds (a compile restoring its own product) is a no-op. It exits 1 when the root is still busy after the wait, and 2 for a bad root or outside a runner job. - A second, different root is refused (exit 2): two jobs taking two roots in opposite orders would deadlock. - `take ROOT --switch` swaps instead. It first lets go of every root the job holds, then waits for ROOT, so - the job never holds two. It only works when each held root has a holder of its own: one taken with - `take`, or the admission root of a `ROOT_SWITCHERS` class (compile-gui, test-e2e's `build`), whose root - the hook hands to a separate holder at admission. A build that finds a product to reuse at another root - switches to it. The caller must be done with the old root. + `take ROOT --switch` swaps instead. + - It waits for ROOT while still holding the old root, and lets the old one go only once ROOT is held. + A timeout (exit 1) leaves the job on its old root, never without one. + - Two switchers after each other's root both time out, so callers keep `--wait` short. + - On success it writes the new `CMUX_CI_CANONICAL_ROOT` to `$GITHUB_ENV`. + - It only works when each held root has a live holder of its own: one taken with `take`, or the + admission root of a `ROOT_SWITCHERS` class (compile-gui, test-e2e's `build`), which the hook hands to + a separate holder at admission. + - A build that finds a product to reuse at another root switches to it. The caller must be done with + the old root. - `glaeda-canonical-root take-gui [--wait S]` holds the mini's gui token from that step to the end of the job, with a third holder (`-gui.pid`) that job-completed releases. It is a no-op when the job already holds gui from admission. It exits 0 when held, 1 when still taken after the wait, and 2 @@ -439,7 +444,8 @@ in at the producer's root. So one root job per root per mini: So every take-root waiter writes `capacity/root-k.want-`, containing `gui` when its job holds the gui token. take-gui exits 3 at once when a gui holder is waiting for a root this job holds. The caller then leaves its console-session work to another job and finishes, which frees the root. - Stale markers (dead pids) are removed on sight. + Waiters rewrite their markers every second. take-gui removes a marker older than 10 s or with a dead + pid, so a killed waiter's leftover cannot make it give way. - Jobs the hook does not know (seed-swiftpm-manifests, anything new) are pinned to root 1, because they use /private/tmp/cmux-ci themselves. Ids that other workflows reuse (`build`, `test`, `lint`) are classed by (workflow file, job id) from `GITHUB_WORKFLOW_REF`, so with `canonicalRoots` above 1 test-e2e's jobs are diff --git a/scripts/glaeda-cmux-runner-hook b/scripts/glaeda-cmux-runner-hook index ae5ee834d..fae489017 100755 --- a/scripts/glaeda-cmux-runner-hook +++ b/scripts/glaeda-cmux-runner-hook @@ -202,6 +202,7 @@ TAKE_ROOT_POLL_S = 1.0 # the lock orders are opposite. So a take-root waiter leaves a marker, capacity/root-k.want- ("gui" when # its job holds the gui token), and take-gui gives way (TAKE_GUI_GAVE_WAY) when a gui holder wants its root. TAKE_GUI_GAVE_WAY = 3 +WANT_FRESH_S = 10.0 # a waiter rewrites its marker every TAKE_ROOT_POLL_S; an older one is stale def canonical_root(k: int) -> str: @@ -648,8 +649,11 @@ def take_root(spec: str, wait_s: float, capacity_dir: Path, state_dir: Path, roo """glaeda-canonical-root take ROOT: hold canonical root ROOT for the rest of this job (cmux#14338), waiting up to wait_s for it. A root this job already holds is a no-op; a second, different root is refused (two jobs taking two roots in opposite orders would deadlock), unless `switch` and every root the job holds - has a holder of its own (ROOT_SWITCHERS, or an earlier take-root): those are let go first, so the job - never holds two. The caller must be done with the old root. Prints the root's path.""" + has a live holder of its own (ROOT_SWITCHERS, or an earlier take-root). A switch waits for ROOT while + still holding the old roots and lets them go only once ROOT is held, so a timeout leaves the job where + it was (exit 1) and never without a root; two switchers after each other's root both time out, so + callers keep --wait short. On success it points $GITHUB_ENV's CMUX_CI_CANONICAL_ROOT at ROOT. Prints + the root's path.""" k = root_number(spec, roots) if k is None: print(f"{PREFIX}: take-root: {spec!r} is not one of this mini's {roots} canonical root(s) " @@ -665,7 +669,7 @@ def take_root(spec: str, wait_s: float, capacity_dir: Path, state_dir: Path, roo if name in held: print(path) return 0 - if held and not (switch and all(root_holder_file(state_dir, old).exists() for old in held)): + if held and not (switch and all(holder_alive(root_holder_file(state_dir, old)) for old in held)): print(f"{PREFIX}: take-root: this job already holds {', '.join(held)}; one root per job" + ("" if switch else " (--switch lets go of a root with its own holder first)"), file=sys.stderr) return 2 @@ -673,12 +677,6 @@ def take_root(spec: str, wait_s: float, capacity_dir: Path, state_dir: Path, roo if worker is None: print(f"{PREFIX}: take-root: not inside a runner job (no Runner.Worker above this step)", file=sys.stderr) return 2 - if held: - for old in held: - release_holder(root_holder_file(state_dir, old)) - with contextlib.suppress(OSError): - roots_file(state_dir).unlink() - print(f"{PREFIX}: take-root: let go of {', '.join(held)}", file=sys.stderr) deadline = time.monotonic() + max(0.0, wait_s) want = capacity_dir / f"{name}.want-{os.getpid()}" try: @@ -689,23 +687,39 @@ def take_root(spec: str, wait_s: float, capacity_dir: Path, state_dir: Path, roo if time.monotonic() >= deadline: print(f"{PREFIX}: take-root: {path} is still in use after {wait_s:g} s", file=sys.stderr) return 1 - if not want.exists(): # take-gui reads it: a gui holder waiting for a root must not be waited on - with contextlib.suppress(OSError): - want.write_text("gui\n" if gui_file(state_dir).exists() else "\n") + # take-gui reads it: a gui holder waiting for a root must not be waited on. Rewritten every poll, + # so a marker a killed waiter left goes stale (WANT_FRESH_S) even if its pid comes back. + with contextlib.suppress(OSError): + want.write_text("gui\n" if gui_file(state_dir).exists() else "\n") time.sleep(TAKE_ROOT_POLL_S) finally: with contextlib.suppress(OSError): want.unlink() - held, note = hold([fd], worker, state_dir, pid_file=root_holder_file(state_dir, name)) - if not held: + ok, note = hold([fd], worker, state_dir, pid_file=root_holder_file(state_dir, name)) + if not ok: print(f"{PREFIX}: take-root: {note}", file=sys.stderr) return 1 - with contextlib.suppress(OSError), open(roots_file(state_dir), "a", encoding="utf-8") as record: - record.write(name + "\n") + if held: # a switch: ROOT is held, so the old roots can go + for old in held: + release_holder(root_holder_file(state_dir, old)) + with contextlib.suppress(OSError): + roots_file(state_dir).write_text(name + "\n") + print(f"{PREFIX}: take-root: let go of {', '.join(held)}; {export_root(name)}", file=sys.stderr) + else: + with contextlib.suppress(OSError), open(roots_file(state_dir), "a", encoding="utf-8") as record: + record.write(name + "\n") print(path) return 0 +def holder_alive(pid_file: Path) -> bool: + """A holder pid file whose holder is still running (a killed one leaves its file behind).""" + try: + return pid_alive(int(pid_file.read_text().strip())) + except (OSError, ValueError): + return False + + def gui_holder_wants(capacity_dir: Path, roots: list[str]) -> str | None: """A root in `roots` that a job holding the gui token is waiting for in take-root, from its marker.""" for name in roots: @@ -714,7 +728,11 @@ def gui_holder_wants(capacity_dir: Path, roots: list[str]) -> str | None: pid, mark = int(marker.name.rsplit("-", 1)[1]), marker.read_text() except (OSError, ValueError): continue - if not pid_alive(pid): + try: + fresh = time.time() - marker.stat().st_mtime < WANT_FRESH_S + except OSError: + continue + if not fresh or not pid_alive(pid): with contextlib.suppress(OSError): marker.unlink() # a waiter that was killed elif mark.strip() == "gui": @@ -758,8 +776,12 @@ def take_gui(wait_s: float, capacity_dir: Path, state_dir: Path) -> int: if not ok: print(f"{PREFIX}: take-gui: {note}", file=sys.stderr) return 1 - with contextlib.suppress(OSError): + try: gui_file(state_dir).write_text("gui\n") + except OSError as error: # unrecorded, a re-take would wait on itself and markers would miss it + release_holder(main.with_name(f"{main.stem}-gui.pid")) + print(f"{PREFIX}: take-gui: cannot record the gui token ({type(error).__name__})", file=sys.stderr) + return 1 return 0 diff --git a/scripts/test-glaeda-cmux-runner.py b/scripts/test-glaeda-cmux-runner.py index ce0c67406..53ba1359c 100644 --- a/scripts/test-glaeda-cmux-runner.py +++ b/scripts/test-glaeda-cmux-runner.py @@ -526,13 +526,13 @@ def test_capacity_admits_by_weight_and_refuses_fast_when_full(self) -> None: self.finish(runner) self.assertTrue(self.lock_free(), "every holder let go") - def take(self, root: str, runner: str, *extra: str) -> subprocess.CompletedProcess: - return self.step(["take-root", "--root", root, "--canonical-roots", "2", *extra], runner) + def take(self, root: str, runner: str, *extra: str, env: dict | None = None) -> subprocess.CompletedProcess: + return self.step(["take-root", "--root", root, "--canonical-roots", "2", *extra], runner, env) def take_gui(self, runner: str, *extra: str) -> subprocess.CompletedProcess: return self.step(["take-gui", *extra], runner) - def step(self, argv: list[str], runner: str) -> subprocess.CompletedProcess: + def step(self, argv: list[str], runner: str, env: dict | None = None) -> subprocess.CompletedProcess: """Run a hook phase as a job step would: under a process named Runner.Worker (its ancestor). A real Runner.Worker outlives the step and the root holder watches it, so the stand-in reports the step's exit status and then stays up until the test ends; one that exited with the step @@ -557,7 +557,7 @@ def step(self, argv: list[str], runner: str) -> subprocess.CompletedProcess: "--capacity-dir", os.fspath(self.dir / "capacity"), "--state-dir", os.fspath(self.dir / "state")]) environ = {"PATH": "/usr/bin:/bin", "HOME": os.fspath(self.dir), "GLAEDA_RUNNER_TELEMETRY": "0", "RUNNER_NAME": runner, - "GLAEDA_FLEET_DIR": os.fspath(self.dir / "fleet")} + "GLAEDA_FLEET_DIR": os.fspath(self.dir / "fleet"), **(env or {})} proc = subprocess.Popen([os.fspath(worker), "-c", cmd], env=environ, stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True) self.addCleanup(self.end_worker, proc) @@ -641,10 +641,19 @@ def test_switch_swaps_a_producers_root_without_holding_two(self) -> None: refused = self.take("2", "e0", "--wait", "0") self.assertEqual(refused.returncode, 2, "without --switch a second root is still refused") self.assertIn("--switch", refused.stderr) - switched = self.take("/private/tmp/cmux-ci-2", "e0", "--switch") + # root 2 busy: a switch that times out keeps the old root, never leaving the job without one + self.assertEqual(self.take("2", "b0").returncode, 0) + late = self.take("2", "e0", "--switch", "--wait", "1") + self.assertEqual(late.returncode, 1, late.stderr) + self.assertEqual(self.take("1", "x0", "--wait", "0").returncode, 1, "e0 still holds root 1") + self.assertEqual((state / "host-lock-holder-e0.roots").read_text().split(), ["root-1"]) + self.finish("b0") + env_file = self.dir / "switch_env" + switched = self.take("/private/tmp/cmux-ci-2", "e0", "--switch", env={"GITHUB_ENV": os.fspath(env_file)}) self.assertEqual((switched.returncode, switched.stdout.strip()), (0, "/private/tmp/cmux-ci-2"), switched.stderr) self.assertIn("let go of root-1", switched.stderr) + self.assertEqual(env_file.read_text(), "CMUX_CI_CANONICAL_ROOT=/private/tmp/cmux-ci-2\n") self.assertEqual((state / "host-lock-holder-e0.roots").read_text().split(), ["root-2"]) self.assertEqual(self.take("1", "c0", "--wait", "5").returncode, 0, "root 1 is free again") gui = self.job("app-host-unit-tests", "g0", 8, None, *two, "--gui-wait", "0") @@ -658,8 +667,10 @@ def test_switch_swaps_a_producers_root_without_holding_two(self) -> None: self.assertIn("persistent-dd+root-1", compile_.stdout) self.assertEqual(self.take("2", "p0", "--switch").returncode, 2, "a root in the admission holder cannot be let go") + (state / "host-lock-holder-p0-root-1.pid").write_text("999999\n") # a killed holder's leftover + self.assertEqual(self.take("2", "p0", "--switch").returncode, 2, "a dead holder lets nothing go") finally: - for runner in ("e0", "c0", "c1", "g0", "p0"): + for runner in ("e0", "c0", "c1", "g0", "p0", "b0", "x0"): self.finish(runner) self.assertFalse(list((self.dir / "capacity").glob("*.want-*")), "no waiter marker outlives its wait") self.assertTrue(self.lock_free()) @@ -719,6 +730,12 @@ def test_take_gui_gives_way_to_a_gui_job_waiting_for_its_root(self) -> None: # a gui holder not waiting for this job's root is no cycle: take-gui keeps waiting for the token self.assertIn("root-2", self.job("macos-compile-admission", "c1", 8, None, *two).stdout) self.assertEqual(self.take_gui("c1", "--wait", "1").returncode, 1) + # a marker its waiter stopped refreshing is stale, even if its pid is alive (recycled or foreign) + stale = self.dir / "capacity" / f"root-2.want-{os.getpid()}" + stale.write_text("gui\n") + os.utime(stale, (time.time() - 60, time.time() - 60)) + self.assertEqual(self.take_gui("c1", "--wait", "1").returncode, 1, "no give-way to a stale marker") + self.assertFalse(stale.exists(), "and it is swept") finally: for runner in ("c0", "g0", "c1"): self.finish(runner) From 1967826ba962f0945cd1c2e4023dd83ac21c709f Mon Sep 17 00:00:00 2001 From: Leo Li Date: Fri, 25 Sep 2026 14:57:01 -0400 Subject: [PATCH 3/3] glaeda-cmux-runner-hook: rewrite the roots record before a switch lets go Re-review follow-up: the record no longer names a released root if the hook dies mid-switch, and the ROOT_SWITCHERS comment matches the new order. Co-Authored-By: Claude Opus 5.5 --- scripts/glaeda-cmux-runner-hook | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/scripts/glaeda-cmux-runner-hook b/scripts/glaeda-cmux-runner-hook index fae489017..23a751954 100755 --- a/scripts/glaeda-cmux-runner-hook +++ b/scripts/glaeda-cmux-runner-hook @@ -194,7 +194,7 @@ CANONICAL_ROOT_ENV = "CMUX_CI_CANONICAL_ROOT" ROOT_CONSUMERS = ("gui", "product") # Producers whose admission root sits in a holder of its own (root_holder_file), so a step can swap it for the # root of a product the job reuses instead of compiling (glaeda-canonical-root take ROOT --switch): the old -# root is let go before the new one is waited for, so the job never holds two roots. +# root is let go once the new one is held, so a switch that times out leaves the job on its old root. ROOT_SWITCHERS = ("compile-gui",) TAKE_ROOT_POLL_S = 1.0 # glaeda-canonical-root take-gui: a job takes the gui token only for the steps that use the console session. @@ -699,11 +699,11 @@ def take_root(spec: str, wait_s: float, capacity_dir: Path, state_dir: Path, roo if not ok: print(f"{PREFIX}: take-root: {note}", file=sys.stderr) return 1 - if held: # a switch: ROOT is held, so the old roots can go - for old in held: - release_holder(root_holder_file(state_dir, old)) + if held: # a switch: ROOT is held, so the old roots can go, after the record stops naming them with contextlib.suppress(OSError): roots_file(state_dir).write_text(name + "\n") + for old in held: + release_holder(root_holder_file(state_dir, old)) print(f"{PREFIX}: take-root: let go of {', '.join(held)}; {export_root(name)}", file=sys.stderr) else: with contextlib.suppress(OSError), open(roots_file(state_dir), "a", encoding="utf-8") as record: