diff --git a/hermes_cli/container_boot.py b/hermes_cli/container_boot.py index cc14d87180b..3d99681a715 100644 --- a/hermes_cli/container_boot.py +++ b/hermes_cli/container_boot.py @@ -379,7 +379,16 @@ def _register_service(scandir: Path, profile: str, *, start: bool) -> None: validate_profile_name(profile) service_dir = scandir / f"gateway-{profile}" - tmp_dir = service_dir.with_name(service_dir.name + ".tmp") + # Dot-prefix the staging dir so s6-svscan skips it while half-built + # (s6-svscan ignores scandir entries whose name starts with "."). + # A non-dotted ``.tmp`` staging name is supervised AS ROOT by any + # concurrent ``s6-svscanctl -a`` rescan the moment it has a valid + # ``type``/``run``, creating a root-owned ``supervise/`` that makes + # ``_seed_supervise_skeleton`` EACCES — see the matching comment in + # ``S6ServiceManager.register_profile_gateway``. The atomic + # ``tmp_dir.replace(service_dir)`` below renames to the dotless live + # name, so the published slot is unchanged. + tmp_dir = service_dir.with_name("." + service_dir.name + ".tmp") # Wipe any leftover tmp from a previous interrupted run. if tmp_dir.exists(): diff --git a/hermes_cli/service_manager.py b/hermes_cli/service_manager.py index 28992a046b1..3cfa6730987 100644 --- a/hermes_cli/service_manager.py +++ b/hermes_cli/service_manager.py @@ -965,9 +965,23 @@ class S6ServiceManager: ) # Build the service directory atomically: write to a sibling - # temp dir, then rename. Avoids s6-svscan observing a half- - # populated directory on a fast rescan. - tmp_dir = svc_dir.with_name(svc_dir.name + ".tmp") + # temp dir, then rename. The staging name is DOT-PREFIXED + # (``.gateway-.tmp``) so s6-svscan ignores it while it + # is half-built: s6-svscan skips any scandir entry whose name + # begins with ``.``. Without the dot prefix, a concurrent + # ``s6-svscanctl -a`` rescan (fired by the cont-init reconciler + # registering ``gateway-default``, or by a sibling register) + # would supervise the still-being-seeded ``.tmp`` slot: it has a + # valid ``type``/``run`` by that point, so s6-supervise spawns + # AS ROOT and mkdir's ``supervise/`` root-owned 0700 — then this + # process's ``_seed_supervise_skeleton`` early-returns on the now- + # existing ``supervise/`` and the next ``mkdir supervise/event`` + # hits EACCES. That is the arm64-only CI flake on + # test_s6_unregister_removes_service_dir_in_live_container + # (the wider scheduling jitter on the native arm64 runner lets the + # rescan land inside the ~ms seed window). The atomic rename to + # the dotless live name below is unaffected. + tmp_dir = svc_dir.with_name("." + svc_dir.name + ".tmp") if tmp_dir.exists(): shutil.rmtree(tmp_dir, ignore_errors=True) tmp_dir.mkdir(parents=True) diff --git a/tests/docker/test_s6_profile_gateway_integration.py b/tests/docker/test_s6_profile_gateway_integration.py index ad1773c7efc..7023fddfbc0 100644 --- a/tests/docker/test_s6_profile_gateway_integration.py +++ b/tests/docker/test_s6_profile_gateway_integration.py @@ -109,3 +109,93 @@ def test_s6_unregister_removes_service_dir_in_live_container( "print(S6ServiceManager().list_profile_gateways())" )) assert "phase3test" not in r.stdout + + +# Shell probe: build a service-shaped staging dir under the live scandir +# with a given NAME, fire a real `s6-svscanctl -a` rescan, wait, and +# report whether s6-svscan supervised it (which would create a root-owned +# supervise/ dir). Used to prove the dot-prefixed staging name is INVISIBLE +# to a concurrent rescan while a non-dotted one is not. +# +# Echoes one of: SUPERVISED / NOT-SUPERVISED, plus the supervise/ owner. +_SVSCAN_PICKUP_PROBE = r""" +set -eu +NAME="$1" +SCANDIR=/run/service +DIR="$SCANDIR/$NAME" +rm -rf "$DIR" +mkdir -p "$DIR" +printf 'longrun\n' > "$DIR/type" +printf '#!/command/execlineb -P\n/command/s6-sleep 600\n' > "$DIR/run" +chmod 755 "$DIR/run" +# Trigger a full rescan, exactly as register/reconcile do. +/command/s6-svscanctl -a "$SCANDIR" +# Give s6-svscan time to act (its scan is async; 200ms is the manager's +# own settle delay, use 2s here to be comfortably past it on any arch). +/command/s6-sleep 2 +if [ -d "$DIR/supervise" ]; then + owner=$(stat -c '%U' "$DIR/supervise" 2>/dev/null || echo '?') + echo "SUPERVISED owner=$owner" +else + echo "NOT-SUPERVISED" +fi +# Best-effort teardown so the probe leaves no live supervisor behind. +/command/s6-svc -d "$DIR" 2>/dev/null || true +/command/s6-svscanctl -an "$SCANDIR" 2>/dev/null || true +/command/s6-sleep 1 +rm -rf "$DIR" 2>/dev/null || true +""" + + +def test_s6_dotfile_staging_dir_is_ignored_by_svscan_rescan( + built_image: str, container_name: str, +) -> None: + """Regression for the arm64 register-seed race. + + The register path builds the slot in a sibling staging dir and then + atomically renames it to the live ``gateway-`` name. That + staging dir lives INSIDE the scandir s6-svscan watches, so its NAME + decides whether a concurrent ``s6-svscanctl -a`` rescan (fired by the + cont-init reconciler registering ``gateway-default``, or by another + register) supervises the half-built slot. + + - A NON-dotted name (the old ``gateway-

.tmp``) IS picked up: once it + has a valid ``type``/``run``, s6-svscan spawns ``s6-supervise`` AS + ROOT, creating a root-owned ``supervise/`` — which makes the in-flight + ``_seed_supervise_skeleton`` EACCES on ``mkdir supervise/event``. That + is the arm64-only flake (the native-arm runner's wider scheduling + jitter lets the rescan land inside the seed window). + - A DOT-prefixed name (the fix, ``.gateway-

.tmp``) is SKIPPED by + s6-svscan and never supervised, so no root-owned ``supervise/`` can + appear under the staging dir. + + This proves the mechanism directly and is arch-independent (it does not + rely on hitting the narrow timing window — it forces the rescan and + checks pickup), so it guards the fix on the amd64 job too. + """ + start_container(built_image, container_name, cmd="sleep 120") + + # Control: a NON-dotted service-shaped dir IS supervised by the rescan + # (root-owned supervise/). This is the pre-fix staging-name behaviour and + # confirms the probe actually exercises s6-svscan pickup. + r = docker_exec( + container_name, "sh", "-c", _SVSCAN_PICKUP_PROBE, "probe", + "gateway-raceprobe.tmp", user="root", timeout=30, + ) + assert "SUPERVISED" in r.stdout and "NOT-SUPERVISED" not in r.stdout, ( + "control failed: a non-dotted staging dir should be picked up by " + f"s6-svscan. stdout={r.stdout!r} stderr={r.stderr!r}" + ) + + # The fix: a DOT-prefixed staging dir (the name register/reconcile now + # use) must be IGNORED by the same rescan — no supervisor, no root-owned + # supervise/, so the in-flight seed can never EACCES. + r = docker_exec( + container_name, "sh", "-c", _SVSCAN_PICKUP_PROBE, "probe", + ".gateway-raceprobe.tmp", user="root", timeout=30, + ) + assert "NOT-SUPERVISED" in r.stdout, ( + "dot-prefixed staging dir was supervised by s6-svscan — the race " + f"that EACCESes the seed is still reachable. stdout={r.stdout!r} " + f"stderr={r.stderr!r}" + ) diff --git a/tests/hermes_cli/test_container_boot.py b/tests/hermes_cli/test_container_boot.py index 165712d2152..9d9981cf6de 100644 --- a/tests/hermes_cli/test_container_boot.py +++ b/tests/hermes_cli/test_container_boot.py @@ -489,19 +489,26 @@ def test_register_service_overwrites_existing_slot(tmp_path: Path) -> None: hermes_home=tmp_path, scandir=scandir, dry_run=False, ) - # Slot still exists, no .tmp remnants. + # Slot still exists, no .tmp remnants (staging dir is dot-prefixed, + # so match it explicitly — a leading-`*` glob won't catch dotfiles). assert (scandir / "gateway-coder" / "run").read_text() == first_run assert list(scandir.glob("*.tmp")) == [] + assert list(scandir.glob(".*.tmp")) == [] # Down marker now present (state went from running → stopped). assert (scandir / "gateway-coder" / "down").exists() def test_register_service_cleans_up_stale_tmp_dir(tmp_path: Path) -> None: - """If a previous interrupted run left a .tmp sibling directory, - a fresh reconcile must clean it up rather than failing on mkdir.""" + """If a previous interrupted run left a staging sibling directory, + a fresh reconcile must clean it up rather than failing on mkdir. + + The staging dir is dot-prefixed (``.gateway-.tmp``) so a + concurrent s6-svscan rescan can't supervise it half-built; the + cleanup must target that same dot-prefixed name. + """ scandir = tmp_path / "run-service"; scandir.mkdir() - # Simulate a leftover from an interrupted run. - stale_tmp = scandir / "gateway-coder.tmp" + # Simulate a leftover from an interrupted run (current staging name). + stale_tmp = scandir / ".gateway-coder.tmp" stale_tmp.mkdir() (stale_tmp / "stale-file").write_text("garbage") diff --git a/tests/hermes_cli/test_service_manager.py b/tests/hermes_cli/test_service_manager.py index cd78c35d55d..21107c248dc 100644 --- a/tests/hermes_cli/test_service_manager.py +++ b/tests/hermes_cli/test_service_manager.py @@ -586,6 +586,52 @@ def test_s6_register_creates_service_dir_and_triggers_scan( ), f"s6-svscanctl -a not invoked; saw: {fake_subprocess_run}" +def test_s6_register_staging_dir_is_dotfile_hidden_from_svscan( + s6_scandir, fake_subprocess_run, monkeypatch: pytest.MonkeyPatch, +) -> None: + """The mid-build staging dir MUST be dot-prefixed so s6-svscan + ignores it while it is half-populated. + + s6-svscan skips any scandir entry whose name begins with ``.``. If + the staging dir were a plain ``gateway-

.tmp`` (a non-dotfile), + a concurrent ``s6-svscanctl -a`` rescan would supervise it the + moment it has a valid ``type``/``run`` — spawning s6-supervise AS + ROOT, which mkdir's a root-owned ``supervise/`` and makes the + in-flight ``_seed_supervise_skeleton`` EACCES on + ``mkdir supervise/event``. That was the arm64-only CI flake on + ``test_s6_unregister_removes_service_dir_in_live_container``. + + We capture the directory passed to ``_seed_supervise_skeleton`` + (called mid-build, BEFORE the atomic rename to the live name) and + assert its basename starts with ``.`` and still lives in the + scandir as a sibling of the live slot. + """ + import hermes_cli.service_manager as sm + + seen: list[str] = [] + real_seed = sm._seed_supervise_skeleton + + def _capturing_seed(svc_dir, *a, **kw): + seen.append(str(svc_dir)) + return real_seed(svc_dir, *a, **kw) + + monkeypatch.setattr(sm, "_seed_supervise_skeleton", _capturing_seed) + + S6ServiceManager(scandir=s6_scandir).register_profile_gateway("coder") + + assert seen, "_seed_supervise_skeleton was never called during register" + staging = seen[0] + staging_name = staging.rsplit("/", 1)[-1] + assert staging_name.startswith("."), ( + f"staging dir must be a dotfile so s6-svscan skips it mid-build; " + f"got {staging_name!r}" + ) + # Sibling of the live slot, in the same scandir. + assert staging == str(s6_scandir / ".gateway-coder.tmp") + # And the published (renamed) live slot is the dotless canonical name. + assert (s6_scandir / "gateway-coder").is_dir() + + def test_s6_register_start_now_false_writes_down_marker( s6_scandir, fake_subprocess_run, ) -> None: