diff --git a/.github/workflows/e2e.yml b/.github/workflows/e2e.yml index c02634f..55d98fa 100644 --- a/.github/workflows/e2e.yml +++ b/.github/workflows/e2e.yml @@ -327,7 +327,9 @@ jobs: # (~5-20x slower; a ~100s+ boot is expected, not a hang — see # docs/design/vm-rnd-log.md). On a KVM-capable runner class accel: auto # would pick KVM for free. The kernel+rootfs build is the long pole; the - # savevm boot-cache (#49) is the planned lever to skip it on repeat runs. + # savevm boot-cache (#49, keyed by scripts/vm_cache_key.py) restores the + # booted overlay and resumes (-loadvm) to skip the cold TCG boot on repeat + # runs. if: >- github.event_name != 'pull_request' || contains(github.event.pull_request.labels.*.name, 'e2e') @@ -399,14 +401,43 @@ jobs: ) PY - - name: "Create + boot a binder:vm instance under QEMU/TCG" + - name: "Create a binder:vm instance (boot_cache on)" run: | set -euxo pipefail uv run beetroot create vmphone - printf 'binder: vm\n' >> vmphone/beetroot.yaml + # boot_cache: true checkpoints the booted guest (savevm) into + # vm-overlay.qcow2 so a downstream job can restore it and resume + # (-loadvm, ~10s) instead of cold-booting under TCG (issue #49). + printf 'binder: vm\nvm:\n boot_cache: true\n' >> vmphone/beetroot.yaml # `apply` switches the registered backend to the micro-VM engine; # `up` refuses a binder:vm instance still registered as redroid. uv run beetroot apply vmphone + + # The cache key is the kernel+rootfs hash (scripts/vm_cache_key.py): a + # checkpoint booted against different artifacts must not be restored, so + # the key changes the instant either input changes (issue #49). + - name: Derive the savevm boot-cache key + id: vmkey + run: | + set -euo pipefail + key="$(uv run python scripts/vm_cache_key.py "$BEETROOT_VM_KERNEL" "$BEETROOT_VM_ROOTFS")" + echo "key=$key" >> "$GITHUB_OUTPUT" + + # Restore a previously checkpointed overlay (resume in seconds) or, on a + # miss, let the cold boot below checkpoint it for next time. The overlay + # carries the booted machine state (a few GiB) — actions/cache compresses + # it and evicts under the ~10GB budget per the issue caveat. + - name: Restore/persist the booted savevm overlay + uses: actions/cache@0400d5f644dc74513175e3cd8d07132dd4860809 # v4.2.4 + with: + path: vmphone/vm-overlay.qcow2 + key: ${{ runner.os }}-vm-savevm-${{ steps.vmkey.outputs.key }} + + - name: "Boot the binder:vm instance under QEMU/TCG" + run: | + set -euxo pipefail + # Resumes from the restored overlay on a cache hit (~10s), else + # cold-boots and checkpoints for the next run. uv run beetroot up vmphone - name: "Wait for Android boot (TCG: slow, not hung)" diff --git a/CHANGELOG.md b/CHANGELOG.md index 70d00d4..4ca0034 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -500,6 +500,13 @@ pre-abort rows in its `results` attribute). ### Quality & internals +- **Fixed self-contradicting `vm.py` comments that claimed `adb connect` succeeds + at QEMU `hostfwd`-bind (#241).** It only attaches once the post-boot in-guest + relay is listening; the comments and the `_wait_for_boot_completed` docstring + now match `guest-init.sh`. +- **The e2e `tier-vm-qemu` job caches the booted `binder: vm` savevm overlay and + resumes it on repeat runs (#49).** Keyed by the kernel+rootfs hash, it skips the + ~100s cold TCG boot for functional/post-boot jobs. - **The supported Android-version list is now drift-checked and the "add a new version" path is documented + tested (#98).** `config._VALID_ANDROID_VERSIONS` has always been the single source of truth, but the human-readable "11, 12, @@ -603,6 +610,46 @@ are absent), so shell regressions are caught locally before the push. ### Bug fixes +- **`binder: vm` `down`/`restart`/`destroy` verify the recorded PID still names + this instance's QEMU before signalling (#162).** A stale or reused pidfile can + no longer SIGTERM/SIGKILL an unrelated process; `is_running()` and the + kill-escalation both route through a `/proc//cmdline` identity check tied + to this instance's argv. +- **`binder: vm` `doctor` no longer reports a healthy VM as broken (#163).** The + upstream-redroid VM guest ships no Magisk, so the `magisk.zygisk` / + `magisk.denylist` health rows (which always failed there) are dropped + alongside the already-skipped `frida.handshake` row. +- **A `binder: vm` `up` that times out waiting for ADB now terminates the QEMU it + just launched (#174).** Previously it orphaned the emulator, trapping the next + `up`; the post-launch waits now tear the guest down on any failure before + re-raising. +- **`binder: vm` boot_cache recreates the qcow2 overlay before a cold boot when + the existing overlay carries no snapshot (#175).** An interrupted first boot no + longer leaves a dirty COW layer that later cold boots accumulate over; the + cache-key sidecar is also written atomically. +- **`binder: vm` boot_cache re-checks QEMU liveness during the ADB wait and falls + back to a single cold boot when a warm `-loadvm` resume dies (#176).** An + unrestorable snapshot no longer burns the full `vm_adb_connect_timeout` with a + misleading TCG-slowness error. +- **The `binder: vm` ADB-connect wait is accelerator-aware (#160).** Under TCG it + uses a boot-completed-scale deadline instead of the flat 60s, so a cold TCG + boot (~222s on Android 14) no longer fails `beetroot up` before the guest + exposes ADB; KVM keeps the short configurable default. +- **`binder: vm` boot_cache folds the resolved `-smp` / `-m` geometry into the + overlay staleness fingerprint (#161).** Editing `vm.smp` / `vm.memory_mib` (or + an `smp: "auto"` host-core change) now invalidates the checkpoint and cold-boots + once instead of `-loadvm`-resuming into a geometry QEMU rejects. +- **`compute_cache_key` / `boot_cache.base_identity` break basename ties on + content hash (#235).** Two inputs sharing a basename now hash to the same key + regardless of argument order, honoring the documented order-independence; each + input is hashed exactly once. +- **The `binder: vm` ADB port is fixed to a single `5555` contract (#237).** The + in-guest relay can no longer diverge from the QEMU `hostfwd` target via a stray + `ADB_TCP_PORT` override. +- **Hardened the `binder: vm` adb relay (#238).** It now targets IPv6 loopback + (dropping the `bindv6only` dependency), verifies `eth0` received `10.0.2.15`, + surfaces sysctl/bring-up failures as distinct warnings, and loops `adbprobe`'s + read so a short read never prints uninitialized bytes. - **The default-rendered `memswap_limit` no longer silently disables container swap (#169).** The bundled compose template defaulted `memswap_limit` to `${MEMSWAP_LIMIT:-${MEM_LIMIT:-3g}}`, so an all-defaults instance resolved to diff --git a/examples/vm.yaml b/examples/vm.yaml index 4d7818e..64fec95 100644 --- a/examples/vm.yaml +++ b/examples/vm.yaml @@ -6,7 +6,10 @@ # # accel: auto prefers KVM when /dev/kvm is available (near-native) and # falls back to TCG (software emulation, ~5-20x slower) otherwise. A slow -# first boot under TCG is expected, not a hang. +# first boot under TCG is expected, not a hang — `up`'s ADB-connect wait is +# accel-aware, so under TCG it waits out the minutes-long cold boot instead of +# timing out (raise BEETROOT_VM_ADB_CONNECT_TIMEOUT only on an unusually slow +# host; issue #160). # # Faster TCG iteration: the default Android version is 14, which cold-boots in # ~190-200 s under TCG; redroid 11 boots in ~100 s (~40% faster) on the same diff --git a/scripts/vm_cache_key.py b/scripts/vm_cache_key.py index 1478f4c..94366f7 100755 --- a/scripts/vm_cache_key.py +++ b/scripts/vm_cache_key.py @@ -65,8 +65,9 @@ def compute_cache_key(paths: list[Path], *, prefix: str = DEFAULT_PREFIX) -> str Compute a stable cache key over a set of input files. The key folds each file's *basename* and content hash into one digest, - sorted by basename so the result is independent of the order the paths are - passed. Including the basename means renaming an input (e.g. swapping which + sorted by ``(basename, content-hash)`` so the result is independent of the + order the paths are passed — even when two inputs share a basename (issue + #235). Including the basename means renaming an input (e.g. swapping which rootfs is staged) changes the key even if two files share content. Args: @@ -83,11 +84,15 @@ def compute_cache_key(paths: list[Path], *, prefix: str = DEFAULT_PREFIX) -> str """ if not paths: raise ValueError("compute_cache_key needs at least one input path") + # Hash each input exactly once (a rootfs is multi-GB; hashing it inside the + # sort key AND again in the fold would double its cost). Precompute the + # {path: digest} map, then sort + fold off it. + digests = {path: hash_file(path) for path in paths} combined = hashlib.sha256() - for path in sorted(paths, key=lambda p: p.name): + for path in sorted(digests, key=lambda p: (p.name, digests[p])): combined.update(path.name.encode()) combined.update(b"\0") - combined.update(hash_file(path).encode()) + combined.update(digests[path].encode()) combined.update(b"\0") return f"{prefix}-{combined.hexdigest()[:_KEY_HEX_LEN]}" diff --git a/src/beetroot/backends/vm.py b/src/beetroot/backends/vm.py index 7a3b750..d095d57 100644 --- a/src/beetroot/backends/vm.py +++ b/src/beetroot/backends/vm.py @@ -54,14 +54,25 @@ _ADB_CONNECT_ATTEMPT_TIMEOUT = 5 # The boot-cache path must checkpoint a *fully booted* guest, so it gates -# ``savevm`` on a real ``getprop sys.boot_completed == 1`` poll rather than the -# plain ``adb connect`` (which succeeds as soon as QEMU's user-net hostfwd binds -# the host port — before the guest's adbd is reachable). The deadline matches +# ``savevm`` on a real ``getprop sys.boot_completed == 1`` poll. That is a +# strictly stronger guarantee than ``_wait_for_adb_connect``: the latter only +# proves the in-guest relay accepts an adb attach, while this confirms Android +# itself reached ``sys.boot_completed`` (guest-init.sh main() starts the relay +# only *after* ``wait_for_boot``, so an accepted connect implies boot, but a +# checkpoint wants the prop read to be certain). The deadline matches # guest-init.sh's own BOOT_TIMEOUT; a cold TCG boot is minutes. _BOOT_COMPLETED_TIMEOUT_SECONDS = 900 _BOOT_COMPLETED_POLL_SECONDS = 3.0 _BOOT_COMPLETED_ATTEMPT_TIMEOUT = 10 +# Floor for the accel-aware ADB-connect deadline under TCG. A cold TCG boot to +# first host ADB is minutes (~222 s for Android 14, vm-rnd-log Stage E), so the +# flat ``settings.vm_adb_connect_timeout`` KVM default (60 s) aborts ``up`` long +# before the guest's relay binds. Under TCG the deadline is raised to a +# boot-completed-scale floor so a slow first boot is waited out, not failed +# (issue #160). KVM keeps the short, configurable default. +_TCG_ADB_CONNECT_FLOOR_SECONDS = _BOOT_COMPLETED_TIMEOUT_SECONDS + # Frida is not yet wired through the QEMU micro-VM (issue #44 scopes the # vm backend to ADB forwarding only): the guest runs redroid with # ``--network none`` and nothing forwards the guest Frida port or @@ -447,7 +458,10 @@ def up(self) -> None: ``binder: vm`` (the row is out of sync — run ``apply``). qemu.QemuLaunchError: On a missing accelerator, missing kernel/rootfs, a launch failure, or if the guest does not - expose ADB within ``settings.vm_adb_connect_timeout`` seconds. + expose ADB within the accel-aware deadline (long under TCG, + ``settings.vm_adb_connect_timeout`` under KVM). On timeout the + just-launched QEMU is terminated so the next ``up`` starts + clean (issue #174). """ if self._cfg.binder != "vm": raise BackendCapabilityError( @@ -462,8 +476,16 @@ def up(self) -> None: self._up_cached(accel) return argv = self.build_argv(accel) - qemu.QemuProcess(self._root).start(argv) - self._wait_for_adb_connect() + proc = qemu.QemuProcess(self._root) + proc.start(argv) + try: + self._wait_for_adb_connect(accel, proc) + except BaseException: + # A timed-out (or otherwise failed) wait must not orphan the QEMU we + # just launched — its live pidfile would trip start()'s + # already-running guard on the next `up` (issue #174). + self.down() + raise def _up_cached(self, accel: qemu.ResolvedAccel) -> None: """ @@ -480,35 +502,55 @@ def _up_cached(self, accel: qemu.ResolvedAccel) -> None: (the next ``up`` just cold-boots again), so a failed checkpoint is a warning, never a hard error. + A warm ``-loadvm`` resume that dies on an unrestorable snapshot is + re-checked for liveness during the ADB wait (issue #176): rather than + burn the full deadline with a misleading TCG-slowness error, the soured + overlay is discarded and the boot retried **exactly once** as a cold + boot. + Args: accel: The resolved accelerator (from :meth:`resolved_accel`). Raises: qemu.QemuLaunchError: On a missing kernel/rootfs, a missing ``qemu-img``, a launch failure, or if the guest does not expose - ADB within ``settings.vm_adb_connect_timeout`` seconds. + ADB within the accel-aware deadline. """ kernel = _resolve_artifact(self._cfg.vm.kernel, settings.vm_kernel, "kernel") base_rootfs = _resolve_artifact(self._cfg.vm.rootfs, settings.vm_rootfs, "rootfs") overlay = boot_cache.overlay_path(self._root) monitor = boot_cache.monitor_path(self._root) + # Resolve the -smp/-m geometry once: it feeds the staleness fingerprint + # (#161 — a geometry edit must invalidate the checkpoint, since QEMU + # rejects a -loadvm into a mismatched geometry), the identity sidecar, + # and the launch argv, which must all agree. + resolved_smp = qemu.resolve_smp(self._cfg.vm.smp) + memory_mib = self._cfg.vm.memory_mib # A stale monitor socket from a prior `down` would block QEMU's bind. monitor.unlink(missing_ok=True) - # Auto-invalidate a checkpoint taken against a now-changed kernel/rootfs - # (#126): resuming a snapshot booted from stale artifacts is worse than - # one cold boot. An overlay with no recorded identity (pre-#126) also - # counts as stale, so it is re-keyed on the next boot. - if overlay.exists() and boot_cache.overlay_is_stale(self._root, kernel, base_rootfs): + # Auto-invalidate a checkpoint taken against now-changed kernel/rootfs or + # -smp/-m geometry (#126/#161): resuming a snapshot booted from stale + # artifacts is worse than one cold boot. An overlay with no recorded + # identity (pre-#126) also counts as stale, so it is re-keyed next boot. + if overlay.exists() and boot_cache.overlay_is_stale( + self._root, kernel, base_rootfs, resolved_smp, memory_mib + ): console.note( f"{self._name!r} boot-cache overlay was built from a different " - "kernel/rootfs; discarding the stale checkpoint and cold-booting " - "once to re-cache." + "kernel/rootfs/geometry; discarding the stale checkpoint and " + "cold-booting once to re-cache." ) boot_cache.discard_overlay(self._root) + elif overlay.exists() and not boot_cache.snapshot_present(overlay): + # The overlay is identity-fresh but carries no snapshot: an aborted + # first cold boot left a dirty COW layer (partial guest writes, no + # checkpoint). Reusing it would cold-boot over soured state, so + # discard it and start the cold boot on a pristine overlay (#175). + boot_cache.discard_overlay(self._root) if not overlay.exists(): boot_cache.create_overlay(base_rootfs, overlay) # Record what this overlay was built from so a later rebuild invalidates it. - boot_cache.record_identity(self._root, kernel, base_rootfs) + boot_cache.record_identity(self._root, kernel, base_rootfs, resolved_smp, memory_mib) warm = boot_cache.snapshot_present(overlay) if warm: console.info(f"resuming cached boot snapshot for {self._name!r} (warm start)") @@ -517,26 +559,59 @@ def _up_cached(self, accel: qemu.ResolvedAccel) -> None: console.info( f"no boot snapshot yet for {self._name!r}; cold-booting once, then caching" ) - argv = qemu.build_qemu_argv( - qemu_bin=settings.qemu_bin, - accel=accel, - kernel=kernel, - rootfs=overlay, - smp=qemu.resolve_smp(self._cfg.vm.smp), - memory_mib=self._cfg.vm.memory_mib, - host_adb_port=ports.well_known(self.ports)["adb"], - disk_format="qcow2", - monitor_socket=monitor, - loadvm=boot_cache.SNAPSHOT_TAG if warm else None, - ) - qemu.QemuProcess(self._root).start(argv) - self._wait_for_adb_connect() + + def _launch(*, loadvm: str | None) -> None: + argv = qemu.build_qemu_argv( + qemu_bin=settings.qemu_bin, + accel=accel, + kernel=kernel, + rootfs=overlay, + smp=resolved_smp, + memory_mib=memory_mib, + host_adb_port=ports.well_known(self.ports)["adb"], + disk_format="qcow2", + monitor_socket=monitor, + loadvm=loadvm, + ) + proc = qemu.QemuProcess(self._root) + proc.start(argv) + try: + # Pass proc so a dead-on-arrival -loadvm resume is caught fast + # (issue #176) instead of burning the full deadline. + self._wait_for_adb_connect(accel, proc) + except BaseException: + # Don't orphan the QEMU we just launched (#174). + self.down() + raise + if warm: - # Resume restores an already-booted guest; nothing to checkpoint. - return + try: + _launch(loadvm=boot_cache.SNAPSHOT_TAG) + except qemu.QemuLaunchError: + # The warm resume died on an unrestorable snapshot. Fall back to + # a single cold boot on a fresh overlay rather than failing the + # `up` outright (#176) — bounded to one retry, no recursion. + console.warn( + f"warm resume for {self._name!r} failed; discarding the " + "snapshot and cold-booting once. See `beetroot logs`." + ) + boot_cache.discard_overlay(self._root) + boot_cache.create_overlay(base_rootfs, overlay) + boot_cache.record_identity( + self._root, kernel, base_rootfs, resolved_smp, memory_mib + ) + warm = False + _launch(loadvm=None) + else: + # Resume restores an already-booted guest; nothing to checkpoint. + return + else: + _launch(loadvm=None) + # Cold boot: the checkpoint must capture a FULLY booted guest, so gate - # on a real boot_completed poll (plain adb connect succeeds the moment - # QEMU's hostfwd binds, long before the guest's adbd is reachable). + # on a real boot_completed poll — a strictly stronger guarantee than the + # adb-connect attach (the relay accepts a connect only once Android has + # booted, but the prop read makes the checkpoint precondition explicit). if not self._wait_for_boot_completed(): console.warn( f"guest {self._name!r} did not reach sys.boot_completed in " @@ -556,9 +631,9 @@ def _wait_for_boot_completed(self) -> bool: """ Poll ``adb shell getprop sys.boot_completed`` until it reads ``1``. - Unlike :meth:`_wait_for_adb_connect` (which is satisfied by QEMU's - hostfwd port binding), this confirms the guest's Android has actually - finished booting and adbd is reachable through the relay — the + Stronger than :meth:`_wait_for_adb_connect` (which is satisfied by the + in-guest relay accepting an adb-connect attach): this confirms the + guest's Android actually read ``sys.boot_completed == 1`` — the precondition for a useful ``savevm`` checkpoint. Returns ``False`` on timeout so the caller can skip the checkpoint rather than snapshot a half-booted guest. @@ -691,7 +766,31 @@ def _warn_on_boot_cache_data_revert(self) -> None: "false in beetroot.yaml to keep /data across restarts." ) - def _wait_for_adb_connect(self) -> None: + @staticmethod + def _adb_connect_deadline_seconds(accel: qemu.ResolvedAccel) -> int: + """ + Resolve the ADB-connect deadline (seconds) for the resolved accelerator. + + ``settings.vm_adb_connect_timeout`` is the KVM (near-native) budget. A + cold TCG boot to first host ADB is minutes (~222 s for Android 14), so + under TCG the deadline is raised to a boot-completed-scale floor + (:data:`_TCG_ADB_CONNECT_FLOOR_SECONDS`) — never *below* the configured + value, so bumping ``BEETROOT_VM_ADB_CONNECT_TIMEOUT`` still wins (issue + #160). + + Args: + accel: The resolved accelerator QEMU launches with. + + Returns: + The deadline in seconds. + """ + if accel == "tcg": + return max(settings.vm_adb_connect_timeout, _TCG_ADB_CONNECT_FLOOR_SECONDS) + return settings.vm_adb_connect_timeout + + def _wait_for_adb_connect( + self, accel: qemu.ResolvedAccel, proc: qemu.QemuProcess | None = None + ) -> None: """ Poll ``adb connect`` against the guest until it accepts or times out. @@ -699,26 +798,47 @@ def _wait_for_adb_connect(self) -> None: adbd to switch it into TCP mode a few seconds *after* boot completes, so the endpoint refuses connections briefly. Retry ``adb connect`` every :data:`_ADB_CONNECT_POLL_SECONDS` until the host adb reports a - successful attach or ``settings.vm_adb_connect_timeout`` elapses. The - happy path (endpoint already up) succeeds on the first attempt and - never sleeps. + successful attach or the accel-aware deadline elapses (long under TCG, + short under KVM — see :meth:`_adb_connect_deadline_seconds`). The happy + path (endpoint already up) succeeds on the first attempt and never + sleeps. + + When ``proc`` is supplied the loop also re-checks QEMU liveness each + round: a ``-loadvm`` that exits in ~1 s on an unrestorable snapshot is + otherwise indistinguishable from a slow boot and would burn the full + deadline. A dead-on-arrival QEMU raises a fast, ``beetroot logs``-pointing + error instead (issue #176). + + Args: + accel: The resolved accelerator (selects the deadline). + proc: The just-launched QEMU handle whose liveness is polled. When + ``None`` the liveness re-check is skipped (cold-boot path). Raises: AdbNotInstalledError: If the ``adb`` binary is not on PATH. - qemu.QemuLaunchError: If no attempt succeeds within the deadline — - a friendly, actionable message (not a traceback). + qemu.QemuLaunchError: If QEMU exits before ADB attaches, or if no + attempt succeeds within the deadline — a friendly, actionable + message (not a traceback). """ if shutil.which(_ADB) is None: raise AdbNotInstalledError("adb not found on PATH (install android-tools)") target = self.adb_address - deadline = time.monotonic() + settings.vm_adb_connect_timeout + budget = self._adb_connect_deadline_seconds(accel) + deadline = time.monotonic() + budget while True: if self._adb_connect_ok(target): return + if proc is not None and not proc.is_running(): + raise qemu.QemuLaunchError( + f"the QEMU micro-VM for {self._name!r} exited before exposing ADB " + f"at {target} (a -loadvm resume onto an unrestorable snapshot, or " + f"a launch failure). Inspect the guest console with " + f"`beetroot logs {self._name}` ({qemu.QemuProcess(self._root).console_log})." + ) if time.monotonic() >= deadline: raise qemu.QemuLaunchError( f"the QEMU micro-VM for {self._name!r} did not expose ADB at " - f"{target} within {settings.vm_adb_connect_timeout}s of launch. " + f"{target} within {budget}s of launch. " "Under TCG software emulation first boot is slow (minutes) — " f"run `beetroot logs {self._name}` to watch the guest boot, give " "it longer (raise BEETROOT_VM_ADB_CONNECT_TIMEOUT), or pin " @@ -844,11 +964,13 @@ def health(self) -> dict[str, CheckResult]: Aggregate the VM-backed health checks for this instance. Includes a VM-specific ``vm.process`` row (is QEMU alive?) and a - ``vm.accel`` row (kvm vs the slow-tcg note), then the shared - adb/magisk rows so downstream tools grep uniformly across backend - kinds. The ``frida.handshake`` row is dropped: Frida is unsupported - on the network-isolated guest (issue #44), so the handshake could - never pass and a permanent ``fail`` row would be noise. + ``vm.accel`` row (kvm vs the slow-tcg note), then the shared adb rows so + downstream tools grep uniformly across backend kinds. Three shared rows + are dropped because they can never pass on the network-isolated, plain + upstream redroid guest: ``frida.handshake`` (Frida is unsupported — + issue #44) and the ``magisk.*`` rows (the ``binder: vm`` guest boots an + unmodified redroid image with no Magisk — issue #163), so a permanent + ``fail`` row would be noise. Returns: Ordered dict of check name → :class:`CheckResult`. @@ -865,6 +987,11 @@ def health(self) -> dict[str, CheckResult]: checks["vm.accel"] = _accel_check(self._cfg.vm.accel) shared = adb_device_health(self) shared.pop("frida.handshake", None) + # The guest runs plain redroid (no Magisk), so the magisk rows would be a + # permanent fail — drop them rather than mislead (issue #163). + for name in list(shared): + if name.startswith("magisk."): + shared.pop(name) checks.update(shared) return checks diff --git a/src/beetroot/settings.py b/src/beetroot/settings.py index 089537b..b4a8331 100644 --- a/src/beetroot/settings.py +++ b/src/beetroot/settings.py @@ -78,8 +78,12 @@ class Settings(BaseSettings): up (default: ``60``). The guest restarts adbd to enable TCP a few seconds *after* ``sys.boot_completed=1``, so the first connect races that late bind; ``up`` retries with backoff until the - endpoint accepts or this deadline elapses. Bump it for slow TCG - first boots. + endpoint accepts or this deadline elapses. This flat default is the + KVM (near-native) budget — under TCG software emulation, where a + cold boot to first ADB is minutes (issue #160), the deadline is + auto-extended to a boot-completed-scale floor so a slow first boot + doesn't abort ``up`` before the guest exposes ADB. Bump it to raise + the floor further on an unusually slow host. """ model_config = SettingsConfigDict( diff --git a/src/beetroot/templates/vm/adbprobe.c b/src/beetroot/templates/vm/adbprobe.c index 6675d11..d65d1d9 100644 --- a/src/beetroot/templates/vm/adbprobe.c +++ b/src/beetroot/templates/vm/adbprobe.c @@ -20,9 +20,22 @@ int main(int argc,char**argv){ unsigned int cmd=0x4e584e43,a0=0x01000001,a1=256*1024; const char*sysid="host::\0"; unsigned len=strlen(sysid)+1; unsigned crc=0; for(unsigned i=0;i/dev/null || true ip addr add 10.0.2.15/24 dev "$_eth" 2>/dev/null || true ip route add default via 10.0.2.2 dev "$_eth" 2>/dev/null || true + # Read the address back: the three commands above swallow errors (the addr + # may already be assigned from a prior boot, which is fine), but if NONE of + # them landed 10.0.2.15 the hostfwd target is unreachable and every host + # `adb connect` silently times out. Surface that as a WARN rather than a + # mute dead relay (issue #238). + if ! ip -o addr show dev "$_eth" 2>/dev/null | grep -q 10.0.2.15; then + log "WARN: $_eth has no 10.0.2.15 address after bring-up; the QEMU" \ + "user-net hostfwd target is unreachable and host adb will time out" \ + "(issue #238)" + fi } wait_for_socket() { @@ -230,7 +246,15 @@ enable_adb_tcp() { # then restarting adbd makes it bind :::${ADB_TCP_PORT} (IPv6 wildcard, which # accepts IPv4 too once bindv6only=0). log "enabling adbd TCP on :${ADB_TCP_PORT} inside redroid" - docker exec redroid sysctl -w net.ipv6.bindv6only=0 >/dev/null 2>&1 || true + # The relay now targets the IPv6 loopback (TCP6:[::1]) so it no longer + # depends on bindv6only=0 to reach the v6-wildcard-bound adbd. We still flip + # the sysctl (cheap; lets a v4-only client work too) but log a distinguishable + # WARN instead of swallowing the result, so a kernel without the knob is + # visible in the boot log rather than silent (issue #238). + if ! docker exec redroid sysctl -w net.ipv6.bindv6only=0 >/dev/null 2>&1; then + log "WARN: could not set net.ipv6.bindv6only=0 in redroid; relay targets" \ + "TCP6:[::1] so this is non-fatal (issue #238)" + fi docker exec redroid setprop service.adb.tcp.port "$ADB_TCP_PORT" 2>/dev/null || true docker exec redroid stop adbd 2>/dev/null || true sleep 1 @@ -268,14 +292,16 @@ start_adb_relay() { # Parameterless wrapper: one inner socat per connection, entering the # container netns and bridging stdio<->adbd. socat EXEC hands the accepted # socket to the child on fd 0/1, so STDIO there IS the host-side connection. - # Target IPv4 loopback (dual-stack adbd accepts it) to avoid v6-scope quirks. + # Target the IPv6 loopback ([::1]): adbd binds the v6 wildcard (:::5555) and + # answers there directly, so we drop the bindv6only=0 dependency the IPv4 + # target needed (vm-rnd-log §C confirms TCP6:[::1] returns CNXN, issue #238). # Write to /run (a tmpfs we mount unconditionally) rather than assuming a # /usr/local/bin exists in the rootfs — and mkdir BEFORE the heredoc. _inner=/run/adb-relay-inner.sh mkdir -p /run cat >"$_inner" < str: return digest.hexdigest() -def base_identity(kernel: Path, rootfs: Path) -> str: +def base_identity(kernel: Path, rootfs: Path, smp: int, memory_mib: int) -> str: """ - Compute a stable digest over the kernel + rootfs an overlay is built from. + Compute a stable digest over the kernel + rootfs + geometry an overlay uses. Mirrors ``scripts/vm_cache_key.compute_cache_key``'s algorithm — each file's - basename plus its streamed SHA-256, folded in basename order so the result - is independent of argument order. A kernel or rootfs rebuild changes the - digest, which is what lets :func:`overlay_is_stale` invalidate a checkpoint - taken against the old artifacts (issue #126 / #49). + basename plus its streamed SHA-256, folded in (basename, content-hash) order + so the result is independent of argument order even when two inputs share a + basename (issue #235). The resolved ``-smp``/``-m`` geometry is folded in too + (as decimal bytes): a ``-loadvm`` resume into a different vCPU/RAM geometry is + rejected by QEMU, so a geometry change must invalidate the checkpoint (issue + #161). A kernel/rootfs rebuild or a geometry edit changes the digest, which is + what lets :func:`overlay_is_stale` invalidate a checkpoint taken against the + old artifacts (issue #126 / #49). Args: kernel: The guest ``bzImage`` the overlay boots. rootfs: The raw rootfs image the overlay backs onto. + smp: The resolved (concrete, not ``"auto"``) vCPU count QEMU launches with. + memory_mib: The guest RAM in MiB QEMU launches with. Returns: A 16-hex-character identity digest. @@ -142,28 +148,47 @@ def base_identity(kernel: Path, rootfs: Path) -> str: Raises: FileNotFoundError: If either input file does not exist. """ + # Hash each input file exactly once (the rootfs is multi-GB; hashing it + # inside the sort key AND again in the fold would double its cost on every + # `up`). Precompute {path: digest}, then sort + fold off that dict. + digests = {path: _hash_file(path) for path in (kernel, rootfs)} combined = hashlib.sha256() - for path in sorted((kernel, rootfs), key=lambda p: p.name): + for path in sorted(digests, key=lambda p: (p.name, digests[p])): combined.update(path.name.encode()) combined.update(b"\0") - combined.update(_hash_file(path).encode()) + combined.update(digests[path].encode()) combined.update(b"\0") + combined.update(str(smp).encode()) + combined.update(b"\0") + combined.update(str(memory_mib).encode()) + combined.update(b"\0") return combined.hexdigest()[:_IDENTITY_HEX_LEN] -def record_identity(instance_dir: Path, kernel: Path, rootfs: Path) -> None: +def record_identity( + instance_dir: Path, kernel: Path, rootfs: Path, smp: int, memory_mib: int +) -> None: """ - Write the overlay's base-identity sidecar (the kernel + rootfs digest). + Write the overlay's base-identity sidecar (kernel + rootfs + geometry digest). - Called when the overlay is (re)created so a later kernel/rootfs change is - detectable. Arguments mirror :func:`base_identity`. + Called when the overlay is (re)created so a later kernel/rootfs change or a + ``-smp``/``-m`` geometry edit is detectable. Arguments mirror + :func:`base_identity`. The sidecar is written atomically (temp file + + ``os.replace``) so an interrupted ``record_identity`` never leaves a + keyless overlay that the next ``up`` would judge stale and discard (issue + #175). Args: instance_dir: The instance directory the sidecar is written into. kernel: The guest ``bzImage`` the overlay boots. rootfs: The raw rootfs image the overlay backs onto. + smp: The resolved vCPU count QEMU launches with. + memory_mib: The guest RAM in MiB QEMU launches with. """ - overlay_key_path(instance_dir).write_text(base_identity(kernel, rootfs), encoding="utf-8") + target = overlay_key_path(instance_dir) + tmp = target.with_name(f"{target.name}.tmp") + tmp.write_text(base_identity(kernel, rootfs, smp, memory_mib), encoding="utf-8") + tmp.replace(target) def read_identity(instance_dir: Path) -> str | None: @@ -182,24 +207,30 @@ def read_identity(instance_dir: Path) -> str | None: return None -def overlay_is_stale(instance_dir: Path, kernel: Path, rootfs: Path) -> bool: +def overlay_is_stale( + instance_dir: Path, kernel: Path, rootfs: Path, smp: int, memory_mib: int +) -> bool: """ - Return True iff the overlay's recorded identity ≠ the current kernel/rootfs. + Return True iff the overlay's recorded identity ≠ the current kernel/rootfs/geometry. A missing/unreadable sidecar (e.g. an overlay built before issue #126) also counts as stale: we can't prove it matches the current artifacts, and - resuming a stale checkpoint is worse than one cold boot. The caller then - discards + recreates the overlay and re-checkpoints. + resuming a stale checkpoint is worse than one cold boot. A ``-smp``/``-m`` + geometry edit flips this too (issue #161) — QEMU rejects a ``-loadvm`` into a + mismatched geometry. The caller then discards + recreates the overlay and + re-checkpoints. Args: instance_dir: The instance directory holding the overlay + sidecar. kernel: The currently-resolved guest kernel. rootfs: The currently-resolved raw rootfs. + smp: The resolved vCPU count QEMU will launch with. + memory_mib: The guest RAM in MiB QEMU will launch with. Returns: ``True`` if the checkpoint should be invalidated, else ``False``. """ - return read_identity(instance_dir) != base_identity(kernel, rootfs) + return read_identity(instance_dir) != base_identity(kernel, rootfs, smp, memory_mib) def discard_overlay(instance_dir: Path) -> None: diff --git a/src/beetroot/vm/qemu.py b/src/beetroot/vm/qemu.py index 8aa42b3..969c474 100644 --- a/src/beetroot/vm/qemu.py +++ b/src/beetroot/vm/qemu.py @@ -373,13 +373,50 @@ def read_pid(self) -> int | None: except ValueError: return None + def _read_proc_cmdline(self, pid: int) -> str | None: + """ + Return ``/proc//cmdline`` as NUL-delimited text, or ``None``. + + The kernel separates argv entries with NUL bytes; we keep them so a + caller can substring-match a whole argument. Any read failure (the + process exited between the liveness probe and this read, or a host + with no ``/proc``) returns ``None``. + """ + try: + return Path(f"/proc/{pid}/cmdline").read_text() + except OSError: + return None + + def _pid_is_qemu(self, pid: int) -> bool: + """ + Return True iff ``pid`` is THIS instance's QEMU process. + + The pidfile is persistent and the kernel recycles PIDs, so a recorded + PID that is merely *live* is not enough — a stale entry can name an + unrelated process that reused the number. We require + ``/proc//cmdline`` to both look like a ``qemu-system`` invocation + and reference this instance's directory (which every path in the argv + built by :func:`build_qemu_argv` lives under), so a reused PID + belonging to anything else reports False and is never signalled + (issue #162). A missing ``/proc`` entry (the process is gone) is also + False. + """ + cmdline = self._read_proc_cmdline(pid) + if cmdline is None: + return False + names_qemu = "qemu-system" in cmdline + names_instance = str(self._instance_dir) in cmdline + return names_qemu and names_instance + def is_running(self) -> bool: """ - Return True iff the recorded PID names a live process. + Return True iff the recorded PID is this instance's live QEMU process. - Probes with ``os.kill(pid, 0)`` — signal 0 performs the existence - and permission check without delivering a signal. A missing pidfile - or a stale PID (process gone) returns False. + Probes with ``os.kill(pid, 0)`` — signal 0 performs the existence and + permission check without delivering a signal — then confirms the PID + still *names* this instance's QEMU via :meth:`_pid_is_qemu`. A missing + pidfile, a stale PID (process gone), or a live-but-reused PID that now + belongs to an unrelated process (issue #162) all return False. """ pid = self.read_pid() if pid is None: @@ -388,9 +425,10 @@ def is_running(self) -> bool: os.kill(pid, 0) except OSError as exc: # ESRCH = no such process (stale pid). EPERM = process exists but - # we can't signal it — still counts as running. - return exc.errno == errno.EPERM - return True + # we can't signal it — still alive, pending the identity check. + if exc.errno != errno.EPERM: + return False + return self._pid_is_qemu(pid) def start(self, argv: list[str]) -> int: """ @@ -438,16 +476,6 @@ def start(self, argv: list[str]) -> int: self.pidfile.write_text(str(proc.pid)) return proc.pid - def _pid_alive(self, pid: int) -> bool: - """ - Return True iff ``pid`` names a live process (signal-0 probe). - """ - try: - os.kill(pid, 0) - except OSError as exc: - return exc.errno == errno.EPERM - return True - def terminate(self) -> bool: """ Terminate the recorded QEMU process and remove the pidfile. @@ -460,13 +488,23 @@ def terminate(self) -> bool: already-dead process is a no-op. The pidfile is removed regardless so a subsequent ``up`` starts fresh. + The recorded PID is verified to still NAME this instance's QEMU + (:meth:`_pid_is_qemu`) before any signal is sent: the pidfile is + persistent and PIDs are recycled, so a stale entry pointing at a + reused PID must never SIGTERM/SIGKILL an unrelated process + (issue #162). A live-but-mismatched PID is left untouched and only + its stale pidfile is cleared. + Returns: - True if a signal was delivered to a live process, False if there - was nothing to terminate. + True if a signal was delivered to this instance's QEMU, False if + there was nothing of ours to terminate. """ pid = self.read_pid() signalled = False - if pid is not None: + # Only signal a PID that still names THIS instance's QEMU. A reused + # PID (or a process we can no longer see in /proc) is left alone — the + # stale pidfile is cleared below. + if pid is not None and self._pid_is_qemu(pid): try: os.kill(pid, signal.SIGTERM) signalled = True @@ -483,20 +521,23 @@ def _escalate_if_alive(self, pid: int) -> None: """ Force-kill ``pid`` if it ignores SIGTERM past the grace window. - Polls liveness every :data:`_TERM_POLL_SECONDS` for up to - :data:`_TERM_GRACE_SECONDS`; the moment the process exits, returns - without escalating. A process still alive at the deadline gets - ``SIGKILL`` (best-effort — a race where it dies between the final - poll and the signal is harmless). + Polls every :data:`_TERM_POLL_SECONDS` for up to + :data:`_TERM_GRACE_SECONDS`; the moment the process is no longer this + instance's QEMU (it exited, or — racing the kernel — its PID was + recycled), returns without escalating. A process still ours at the + deadline gets ``SIGKILL`` (best-effort — a race where it dies between + the final poll and the signal is harmless). Re-checking identity + (:meth:`_pid_is_qemu`) rather than bare liveness keeps the SIGKILL + from landing on a reused PID (issue #162). Args: pid: The PID already sent ``SIGTERM``. """ deadline = time.monotonic() + _TERM_GRACE_SECONDS while time.monotonic() < deadline: - if not self._pid_alive(pid): + if not self._pid_is_qemu(pid): return time.sleep(_TERM_POLL_SECONDS) - if self._pid_alive(pid): + if self._pid_is_qemu(pid): with contextlib.suppress(OSError): os.kill(pid, signal.SIGKILL) diff --git a/tests/test_qemu.py b/tests/test_qemu.py new file mode 100644 index 0000000..2b0e30d --- /dev/null +++ b/tests/test_qemu.py @@ -0,0 +1,40 @@ +"""Guards on the fixed `binder: vm` ADB-port contract (issue #237). + +The in-guest relay (``guest-init.sh``) and the host-side QEMU ``hostfwd`` +target must agree on a single guest ADB port, ``5555``. ``guest-init.sh`` pins +``ADB_TCP_PORT=5555`` (non-overridable); this test asserts the Python side +emits the same number, so a future divergence between the documented guest +contract and the ``hostfwd`` target fails CI. +""" + +from __future__ import annotations + +from pathlib import Path + +from beetroot.vm import qemu + + +def _argv(**over: object) -> list[str]: + kwargs: dict[str, object] = { + "qemu_bin": "qemu-system-x86_64", + "accel": "tcg", + "kernel": Path("/img/bzImage"), + "rootfs": Path("/img/rootdisk.img"), + "smp": 4, + "memory_mib": 8192, + "host_adb_port": 5575, + } + kwargs.update(over) + return qemu.build_qemu_argv(**kwargs) # type: ignore[arg-type] + + +def test_hostfwd_guest_slot_matches_guest_adb_port_contract() -> None: + argv = _argv(host_adb_port=5575) + netdev = argv[argv.index("-netdev") + 1] + # netdev looks like: user,id=net0,hostfwd=tcp:127.0.0.1:5575-:5555 — the + # guest slot (after the final ':') must equal _GUEST_ADB_PORT, the same + # fixed 5555 the guest-init relay binds (issue #237). + hostfwd = next(part for part in netdev.split(",") if part.startswith("hostfwd=")) + guest_port = int(hostfwd.rsplit(":", 1)[1]) + assert guest_port == qemu._GUEST_ADB_PORT + assert qemu._GUEST_ADB_PORT == 5555 diff --git a/tests/test_vm_backend.py b/tests/test_vm_backend.py index 5ea182f..2acb459 100644 --- a/tests/test_vm_backend.py +++ b/tests/test_vm_backend.py @@ -307,7 +307,7 @@ def _start(_self: object, argv: list[str]) -> int: return 1234 monkeypatch.setattr(qemu.QemuProcess, "start", _start) - monkeypatch.setattr(vm_backend.VmDeviceBackend, "_wait_for_adb_connect", lambda _self: None) + monkeypatch.setattr(vm_backend.VmDeviceBackend, "_wait_for_adb_connect", lambda _self, *_a: None) backend.up() assert launched["argv"][0] == "qemu-system-x86_64" @@ -353,7 +353,7 @@ def _start(_self: object, argv: list[str]) -> int: return 1 monkeypatch.setattr(qemu.QemuProcess, "start", _start) - monkeypatch.setattr(vm_backend.VmDeviceBackend, "_wait_for_adb_connect", lambda _self: None) + monkeypatch.setattr(vm_backend.VmDeviceBackend, "_wait_for_adb_connect", lambda _self, *_a: None) backend.up() argv = launched["argv"] assert argv[0] == "qemu-system-x86_64" @@ -375,6 +375,43 @@ def test_up_propagates_accel_error( with pytest.raises(qemu.QemuLaunchError, match="/dev/kvm is absent"): backend.up() + def test_up_adb_timeout_terminates_qemu( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + # #174: a timed-out adb-connect wait must terminate the just-launched + # QEMU (via down()) instead of orphaning it for the next `up` to trip on. + _stage_artifacts(monkeypatch, tmp_path) + backend = _make_backend(tmp_path) + monkeypatch.setattr(qemu, "detect_accel", lambda _req: "tcg") + monkeypatch.setattr(qemu.QemuProcess, "start", lambda _self, _argv: 4321) + + def _boom(_self: object, _accel: str, _proc: object) -> None: + raise qemu.QemuLaunchError("did not expose ADB") + + downs: list[str] = [] + monkeypatch.setattr(vm_backend.VmDeviceBackend, "_wait_for_adb_connect", _boom) + monkeypatch.setattr(backend, "down", lambda: downs.append("down")) + with pytest.raises(qemu.QemuLaunchError, match="did not expose ADB"): + backend.up() + assert downs == ["down"] # terminated exactly once + + def test_up_happy_path_does_not_terminate( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + # The leak guard (#174) must not fire when the wait succeeds — a healthy + # boot leaves QEMU running. + _stage_artifacts(monkeypatch, tmp_path) + backend = _make_backend(tmp_path) + monkeypatch.setattr(qemu, "detect_accel", lambda _req: "tcg") + monkeypatch.setattr(qemu.QemuProcess, "start", lambda _self, _argv: 4321) + monkeypatch.setattr( + vm_backend.VmDeviceBackend, "_wait_for_adb_connect", lambda _self, *_a: None + ) + monkeypatch.setattr( + backend, "down", lambda: (_ for _ in ()).throw(AssertionError("down must not run")) + ) + backend.up() + def _cached_backend( self, tmp_path: Path, @@ -389,7 +426,7 @@ def _cached_backend( ) backend = _make_backend(tmp_path, cfg=cfg) monkeypatch.setattr(qemu, "detect_accel", lambda _req: "tcg") - monkeypatch.setattr(vm_backend.VmDeviceBackend, "_wait_for_adb_connect", lambda _self: None) + monkeypatch.setattr(vm_backend.VmDeviceBackend, "_wait_for_adb_connect", lambda _self, *_a: None) # Default: the guest boots (cold path gates savevm on this). The # not-booted branch is exercised explicitly below. monkeypatch.setattr( @@ -406,6 +443,11 @@ def _artifacts(backend: vm_backend.VmDeviceBackend) -> tuple[Path, Path]: assert rootfs is not None return Path(kernel), Path(rootfs) + @staticmethod + def _geometry(backend: vm_backend.VmDeviceBackend) -> tuple[int, int]: + # The resolved (-smp, -m) the backend folds into the overlay identity. + return qemu.resolve_smp(backend._cfg.vm.smp), backend._cfg.vm.memory_mib + def test_up_cached_cold_creates_overlay_and_checkpoints( self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: @@ -446,7 +488,8 @@ def test_up_cached_warm_resumes_without_recheckpoint( # -loadvm and do NOT re-checkpoint. boot_cache.overlay_path(backend.root).write_bytes(b"qcow2") kernel, rootfs = self._artifacts(backend) - boot_cache.record_identity(backend.root, kernel, rootfs) + smp, mem = self._geometry(backend) + boot_cache.record_identity(backend.root, kernel, rootfs, smp, mem) events: list[str] = [] launched: dict[str, list[str]] = {} monkeypatch.setattr( @@ -554,7 +597,138 @@ def test_up_cached_invalidates_stale_overlay( # and the sidecar re-keyed to the current kernel/rootfs. assert any("discarding the stale checkpoint" in n for n in notes) assert "overlay" in events - assert boot_cache.read_identity(backend.root) == boot_cache.base_identity(kernel, rootfs) + smp, mem = self._geometry(backend) + assert boot_cache.read_identity(backend.root) == boot_cache.base_identity( + kernel, rootfs, smp, mem + ) + + def test_up_cached_geometry_change_invalidates_overlay( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + # #161: an overlay keyed to the SAME kernel/rootfs but a DIFFERENT + # -smp/-m geometry must be discarded + cold-booted, not -loadvm-resumed + # into a geometry QEMU rejects. + backend = self._cached_backend(tmp_path, monkeypatch) + kernel, rootfs = self._artifacts(backend) + smp, mem = self._geometry(backend) + boot_cache.overlay_path(backend.root).write_bytes(b"qcow2") + # Record an identity for a DIFFERENT memory geometry than the config. + boot_cache.record_identity(backend.root, kernel, rootfs, smp, mem + 4096) + discarded: list[str] = [] + launched: dict[str, list[str]] = {} + monkeypatch.setattr("beetroot.vm.boot_cache.create_overlay", lambda *_a: None) + + def _discard(root: Path) -> None: + discarded.append("discard") + boot_cache.overlay_path(root).unlink(missing_ok=True) + + monkeypatch.setattr("beetroot.vm.boot_cache.discard_overlay", _discard) + monkeypatch.setattr("beetroot.vm.boot_cache.snapshot_present", lambda _o: False) + monkeypatch.setattr("beetroot.vm.boot_cache.save_snapshot", lambda _m: True) + + def _start(_self: object, argv: list[str]) -> int: + launched["argv"] = argv + return 1 + + monkeypatch.setattr(qemu.QemuProcess, "start", _start) + backend.up() + assert discarded == ["discard"] + assert "-loadvm" not in launched["argv"] # cold boot, not a resume + + def test_up_cached_aborted_first_boot_recreates_dirty_overlay( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + # #175: an identity-fresh overlay that carries NO snapshot is the + # residue of an aborted first cold boot (a dirty COW layer). It must be + # discarded and recreated so the next cold boot starts pristine, not + # accumulate over the soured layer. + backend = self._cached_backend(tmp_path, monkeypatch) + kernel, rootfs = self._artifacts(backend) + smp, mem = self._geometry(backend) + boot_cache.overlay_path(backend.root).write_bytes(b"dirty-qcow2") + # Identity matches the current artifacts → NOT stale; but no snapshot. + boot_cache.record_identity(backend.root, kernel, rootfs, smp, mem) + events: list[str] = [] + + def _discard(root: Path) -> None: + events.append("discard") + boot_cache.overlay_path(root).unlink(missing_ok=True) + + monkeypatch.setattr("beetroot.vm.boot_cache.discard_overlay", _discard) + monkeypatch.setattr( + "beetroot.vm.boot_cache.create_overlay", lambda *_a: events.append("create") + ) + monkeypatch.setattr("beetroot.vm.boot_cache.snapshot_present", lambda _o: False) + monkeypatch.setattr("beetroot.vm.boot_cache.save_snapshot", lambda _m: True) + monkeypatch.setattr(qemu.QemuProcess, "start", lambda _self, _argv: 1) + notes: list[str] = [] + monkeypatch.setattr("beetroot.console.note", notes.append) + backend.up() + # Discarded the dirty overlay, then recreated a pristine one before launch. + assert events == ["discard", "create"] + # Not a stale-identity discard — no "different kernel/rootfs" note. + assert not any("discarding the stale checkpoint" in n for n in notes) + + def test_up_cached_warm_snapshot_present_overlay_kept( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + # Complement to #175: an overlay WITH a snapshot (a real warm cache) is + # neither discarded nor recreated — the warm path is untouched. + backend = self._cached_backend(tmp_path, monkeypatch) + kernel, rootfs = self._artifacts(backend) + smp, mem = self._geometry(backend) + boot_cache.overlay_path(backend.root).write_bytes(b"qcow2") + boot_cache.record_identity(backend.root, kernel, rootfs, smp, mem) + events: list[str] = [] + monkeypatch.setattr( + "beetroot.vm.boot_cache.discard_overlay", lambda _root: events.append("discard") + ) + monkeypatch.setattr( + "beetroot.vm.boot_cache.create_overlay", lambda *_a: events.append("create") + ) + monkeypatch.setattr("beetroot.vm.boot_cache.snapshot_present", lambda _o: True) + monkeypatch.setattr(qemu.QemuProcess, "start", lambda _self, _argv: 1) + backend.up() + assert events == [] # warm overlay left intact + + def test_up_cached_warm_resume_dies_falls_back_to_cold( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + # #176: a warm -loadvm resume whose QEMU dies on an unrestorable + # snapshot is discarded and cold-booted ONCE (loadvm=None), instead of + # failing the `up` outright. + backend = self._cached_backend(tmp_path, monkeypatch) + kernel, rootfs = self._artifacts(backend) + smp, mem = self._geometry(backend) + boot_cache.overlay_path(backend.root).write_bytes(b"qcow2") + boot_cache.record_identity(backend.root, kernel, rootfs, smp, mem) + monkeypatch.setattr("beetroot.vm.boot_cache.snapshot_present", lambda _o: True) + monkeypatch.setattr("beetroot.vm.boot_cache.create_overlay", lambda *_a: None) + monkeypatch.setattr("beetroot.vm.boot_cache.discard_overlay", lambda _root: None) + monkeypatch.setattr("beetroot.vm.boot_cache.save_snapshot", lambda _m: True) + launched: list[list[str]] = [] + + def _start(_self: object, argv: list[str]) -> int: + launched.append(argv) + return 1 + + monkeypatch.setattr(qemu.QemuProcess, "start", _start) + # The warm wait (first call) dies; the cold retry (second) succeeds. + calls: list[str] = [] + + def _wait(_self: object, _accel: str, _proc: object = None) -> None: + calls.append("wait") + if len(calls) == 1: + raise qemu.QemuLaunchError("exited before exposing ADB") + + monkeypatch.setattr(vm_backend.VmDeviceBackend, "_wait_for_adb_connect", _wait) + warnings: list[str] = [] + monkeypatch.setattr("beetroot.console.warn", warnings.append) + backend.up() + assert len(launched) == 2 # warm attempt, then one cold retry + assert "-loadvm" in launched[0] # warm tried -loadvm + assert "-loadvm" not in launched[1] # cold retry did not + assert any("warm resume" in w and "cold-booting" in w for w in warnings) def test_up_cached_cold_records_identity( self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch @@ -568,7 +742,10 @@ def test_up_cached_cold_records_identity( monkeypatch.setattr("beetroot.vm.boot_cache.save_snapshot", lambda _m: True) monkeypatch.setattr(qemu.QemuProcess, "start", lambda _self, _argv: 1) backend.up() - assert boot_cache.read_identity(backend.root) == boot_cache.base_identity(kernel, rootfs) + smp, mem = self._geometry(backend) + assert boot_cache.read_identity(backend.root) == boot_cache.base_identity( + kernel, rootfs, smp, mem + ) def test_up_cached_cold_skips_checkpoint_when_boot_times_out( self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch @@ -699,7 +876,7 @@ def _backend_with_rootfs( def _silence_launch(self, monkeypatch: pytest.MonkeyPatch) -> None: monkeypatch.setattr(qemu, "detect_accel", lambda _req: "tcg") monkeypatch.setattr(qemu.QemuProcess, "start", lambda _self, _argv: 1) - monkeypatch.setattr(vm_backend.VmDeviceBackend, "_wait_for_adb_connect", lambda _self: None) + monkeypatch.setattr(vm_backend.VmDeviceBackend, "_wait_for_adb_connect", lambda _self, *_a: None) def test_up_warns_on_version_mismatch( self, @@ -893,7 +1070,7 @@ def test_up_does_not_emit_inert_warning( backend = _make_backend(tmp_path, cfg=cfg) monkeypatch.setattr(qemu, "detect_accel", lambda _req: "tcg") monkeypatch.setattr(qemu.QemuProcess, "start", lambda _self, _argv: 1) - monkeypatch.setattr(vm_backend.VmDeviceBackend, "_wait_for_adb_connect", lambda _self: None) + monkeypatch.setattr(vm_backend.VmDeviceBackend, "_wait_for_adb_connect", lambda _self, *_a: None) backend.up() err = capsys.readouterr().err assert "android.gapps: full" not in err @@ -1018,7 +1195,7 @@ def _run(cmd: list[str], **_k: object) -> subprocess.CompletedProcess[str]: sleeps: list[float] = [] monkeypatch.setattr("beetroot.backends.vm.subprocess.run", _run) monkeypatch.setattr("beetroot.backends.vm.time.sleep", sleeps.append) - backend._wait_for_adb_connect() + backend._wait_for_adb_connect("kvm") assert len(attempts) == 3 assert attempts[0] == ["adb", "connect", "localhost:5555"] # Two refusals → two backoff sleeps before the third (winning) attempt. @@ -1040,7 +1217,7 @@ def _no_sleep(_s: float) -> None: monkeypatch.setattr("beetroot.backends.vm.subprocess.run", _run) monkeypatch.setattr("beetroot.backends.vm.time.sleep", _no_sleep) - backend._wait_for_adb_connect() + backend._wait_for_adb_connect("kvm") assert len(attempts) == 1 def test_never_connects_raises_friendly_error( @@ -1058,7 +1235,7 @@ def test_never_connects_raises_friendly_error( monkeypatch.setattr("beetroot.backends.vm.time.monotonic", lambda: next(ticks)) monkeypatch.setattr("beetroot.backends.vm.time.sleep", lambda _s: None) with pytest.raises(qemu.QemuLaunchError, match="did not expose ADB"): - backend._wait_for_adb_connect() + backend._wait_for_adb_connect("kvm") def test_subprocess_error_treated_as_not_connected( self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch @@ -1075,7 +1252,47 @@ def test_without_adb_raises(self, tmp_path: Path, monkeypatch: pytest.MonkeyPatc backend = _make_backend(tmp_path) monkeypatch.setattr(shutil, "which", lambda _n: None) with pytest.raises(api.AdbNotInstalledError): - backend._wait_for_adb_connect() + backend._wait_for_adb_connect("kvm") + + def test_deadline_is_accel_aware(self, monkeypatch: pytest.MonkeyPatch) -> None: + # #160: under TCG (cold boot ~minutes) the adb-connect deadline is + # raised to a boot-completed-scale floor; under KVM it stays the short, + # configurable default. Cover BOTH branches. + monkeypatch.setattr(vm_backend, "settings", Settings(vm_adb_connect_timeout=60)) + tcg = vm_backend.VmDeviceBackend._adb_connect_deadline_seconds("tcg") + kvm = vm_backend.VmDeviceBackend._adb_connect_deadline_seconds("kvm") + assert kvm == 60 + assert tcg >= vm_backend._BOOT_COMPLETED_TIMEOUT_SECONDS + assert tcg > kvm + + def test_tcg_deadline_never_below_configured(self, monkeypatch: pytest.MonkeyPatch) -> None: + # A user bumping BEETROOT_VM_ADB_CONNECT_TIMEOUT above the floor still + # wins — the floor is a max(), never a cap (#160). + huge = vm_backend._TCG_ADB_CONNECT_FLOOR_SECONDS + 1000 + monkeypatch.setattr(vm_backend, "settings", Settings(vm_adb_connect_timeout=huge)) + assert vm_backend.VmDeviceBackend._adb_connect_deadline_seconds("tcg") == huge + + def test_dead_qemu_raises_fast_with_logs_pointer( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + # #176: a -loadvm resume that exits in ~1s is caught by the liveness + # re-check and raised immediately (pointing at `beetroot logs`), not + # waited out to the full deadline. + backend = _make_backend(tmp_path) + monkeypatch.setattr(shutil, "which", lambda name: f"/usr/bin/{name}") + monkeypatch.setattr( + "beetroot.backends.vm.subprocess.run", + lambda *_a, **_k: self._connect_result(ok=False), + ) + + def _no_sleep(_s: float) -> None: + raise AssertionError("a dead QEMU must raise before any backoff sleep") + + monkeypatch.setattr("beetroot.backends.vm.time.sleep", _no_sleep) + proc = qemu.QemuProcess(backend.root) + monkeypatch.setattr(qemu.QemuProcess, "is_running", lambda _self: False) + with pytest.raises(qemu.QemuLaunchError, match="exited before exposing ADB"): + backend._wait_for_adb_connect("tcg", proc) def test_up_full_path_launches_then_waits_for_adb( self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch @@ -1086,6 +1303,9 @@ def test_up_full_path_launches_then_waits_for_adb( backend = _make_backend(tmp_path) monkeypatch.setattr(qemu, "detect_accel", lambda _req: "tcg") monkeypatch.setattr(qemu.QemuProcess, "start", lambda _self, _argv: 4321) + # The launched QEMU stays alive across the early connect refusals so the + # liveness re-check (issue #176) doesn't short-circuit the retry loop. + monkeypatch.setattr(qemu.QemuProcess, "is_running", lambda _self: True) monkeypatch.setattr(shutil, "which", lambda name: f"/usr/bin/{name}") attempts: list[list[str]] = [] @@ -1116,11 +1336,14 @@ def test_health_includes_vm_rows(self, tmp_path: Path, monkeypatch: pytest.Monke assert rows["vm.process"].status == "pass" assert rows["vm.accel"].status == "pass" assert "near-native" in (rows["vm.accel"].reason or "") - # Shared adb/magisk rows are present (uniform check names). - assert "magisk.zygisk" in rows + # The shared adb.serial row is present (uniform check names). + assert "adb.serial" in rows # Frida can never pass on the network-isolated guest (#44), so the # handshake row is omitted rather than a permanent fail. assert "frida.handshake" not in rows + # The guest runs plain redroid with no Magisk (#163), so the magisk + # rows are dropped rather than reported as a permanent fail. + assert not any(name.startswith("magisk.") for name in rows) def test_health_process_down_fails( self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch diff --git a/tests/test_vm_boot_cache.py b/tests/test_vm_boot_cache.py index 41336ff..244dc3a 100644 --- a/tests/test_vm_boot_cache.py +++ b/tests/test_vm_boot_cache.py @@ -291,21 +291,59 @@ def test_overlay_key_path(self, tmp_path: Path) -> None: def test_base_identity_stable_and_order_independent(self, tmp_path: Path) -> None: kernel, rootfs = self._files(tmp_path) - first = boot_cache.base_identity(kernel, rootfs) + first = boot_cache.base_identity(kernel, rootfs, 4, 8192) # Argument order must not matter (folded in basename order). - assert first == boot_cache.base_identity(rootfs, kernel) + assert first == boot_cache.base_identity(rootfs, kernel, 4, 8192) assert len(first) == 16 def test_base_identity_changes_with_content(self, tmp_path: Path) -> None: kernel, rootfs = self._files(tmp_path) - before = boot_cache.base_identity(kernel, rootfs) + before = boot_cache.base_identity(kernel, rootfs, 4, 8192) kernel.write_bytes(b"REBUILT KERNEL") - assert boot_cache.base_identity(kernel, rootfs) != before + assert boot_cache.base_identity(kernel, rootfs, 4, 8192) != before + + def test_base_identity_changes_with_smp(self, tmp_path: Path) -> None: + # #161: a -smp geometry change must alter the fingerprint, since QEMU + # rejects a -loadvm resume into a mismatched vCPU count. + kernel, rootfs = self._files(tmp_path) + assert boot_cache.base_identity(kernel, rootfs, 8, 8192) != boot_cache.base_identity( + kernel, rootfs, 4, 8192 + ) + + def test_base_identity_changes_with_memory(self, tmp_path: Path) -> None: + # #161: a -m (RAM) geometry change must alter the fingerprint too. + kernel, rootfs = self._files(tmp_path) + assert boot_cache.base_identity(kernel, rootfs, 4, 8192) != boot_cache.base_identity( + kernel, rootfs, 4, 4096 + ) + + def test_base_identity_order_independent_with_colliding_basenames(self, tmp_path: Path) -> None: + # #235: two inputs sharing a basename must hash to the same key + # regardless of argument order (the tie breaks on content hash). + a = tmp_path / "dirA" + a.mkdir() + b = tmp_path / "dirB" + b.mkdir() + ka = a / "bzImage" + ka.write_bytes(b"KERNEL-X") + kb = b / "bzImage" + kb.write_bytes(b"KERNEL-Y") + assert boot_cache.base_identity(ka, kb, 4, 8192) == boot_cache.base_identity(kb, ka, 4, 8192) def test_record_and_read_roundtrip(self, tmp_path: Path) -> None: kernel, rootfs = self._files(tmp_path) - boot_cache.record_identity(tmp_path, kernel, rootfs) - assert boot_cache.read_identity(tmp_path) == boot_cache.base_identity(kernel, rootfs) + boot_cache.record_identity(tmp_path, kernel, rootfs, 4, 8192) + assert boot_cache.read_identity(tmp_path) == boot_cache.base_identity( + kernel, rootfs, 4, 8192 + ) + + def test_record_identity_is_atomic_no_temp_left(self, tmp_path: Path) -> None: + # #175: the sidecar is written via temp + replace, so no .tmp file is + # left behind and the final write is all-or-nothing. + kernel, rootfs = self._files(tmp_path) + boot_cache.record_identity(tmp_path, kernel, rootfs, 4, 8192) + assert not (tmp_path / "vm-overlay.cache-key.tmp").exists() + assert boot_cache.read_identity(tmp_path) is not None def test_read_identity_none_when_absent(self, tmp_path: Path) -> None: assert boot_cache.read_identity(tmp_path) is None @@ -321,23 +359,30 @@ def test_read_identity_none_on_oserror(self, tmp_path: Path) -> None: def test_overlay_is_stale_false_when_matching(self, tmp_path: Path) -> None: kernel, rootfs = self._files(tmp_path) - boot_cache.record_identity(tmp_path, kernel, rootfs) - assert boot_cache.overlay_is_stale(tmp_path, kernel, rootfs) is False + boot_cache.record_identity(tmp_path, kernel, rootfs, 4, 8192) + assert boot_cache.overlay_is_stale(tmp_path, kernel, rootfs, 4, 8192) is False def test_overlay_is_stale_true_when_content_changed(self, tmp_path: Path) -> None: kernel, rootfs = self._files(tmp_path) - boot_cache.record_identity(tmp_path, kernel, rootfs) + boot_cache.record_identity(tmp_path, kernel, rootfs, 4, 8192) rootfs.write_bytes(b"REBUILT ROOTFS") - assert boot_cache.overlay_is_stale(tmp_path, kernel, rootfs) is True + assert boot_cache.overlay_is_stale(tmp_path, kernel, rootfs, 4, 8192) is True + + def test_overlay_is_stale_true_when_geometry_changed(self, tmp_path: Path) -> None: + # #161: an unchanged kernel/rootfs but a different -smp/-m must read stale. + kernel, rootfs = self._files(tmp_path) + boot_cache.record_identity(tmp_path, kernel, rootfs, 8, 8192) + assert boot_cache.overlay_is_stale(tmp_path, kernel, rootfs, 4, 8192) is True + assert boot_cache.overlay_is_stale(tmp_path, kernel, rootfs, 8, 4096) is True def test_overlay_is_stale_true_when_no_key(self, tmp_path: Path) -> None: kernel, rootfs = self._files(tmp_path) - assert boot_cache.overlay_is_stale(tmp_path, kernel, rootfs) is True + assert boot_cache.overlay_is_stale(tmp_path, kernel, rootfs, 4, 8192) is True def test_discard_overlay_removes_overlay_and_key(self, tmp_path: Path) -> None: kernel, rootfs = self._files(tmp_path) boot_cache.overlay_path(tmp_path).write_bytes(b"q") - boot_cache.record_identity(tmp_path, kernel, rootfs) + boot_cache.record_identity(tmp_path, kernel, rootfs, 4, 8192) boot_cache.discard_overlay(tmp_path) assert not boot_cache.overlay_path(tmp_path).exists() assert not boot_cache.overlay_key_path(tmp_path).exists() diff --git a/tests/test_vm_cache_key.py b/tests/test_vm_cache_key.py index 58d812e..e853386 100644 --- a/tests/test_vm_cache_key.py +++ b/tests/test_vm_cache_key.py @@ -47,6 +47,19 @@ def test_key_is_order_independent(tmp_path: Path) -> None: assert vm_cache_key.compute_cache_key([a, b]) == vm_cache_key.compute_cache_key([b, a]) +def test_key_order_independent_with_colliding_basenames(tmp_path: Path) -> None: + # #235: two inputs sharing a basename but with different content must hash + # to the same key regardless of argument order (the tie breaks on content + # hash, not on the input order the stable sort would otherwise preserve). + dir_a = tmp_path / "dirA" + dir_a.mkdir() + dir_b = tmp_path / "dirB" + dir_b.mkdir() + a = _write(dir_a / "kernel.config", b"X") + b = _write(dir_b / "kernel.config", b"Y") + assert vm_cache_key.compute_cache_key([a, b]) == vm_cache_key.compute_cache_key([b, a]) + + def test_key_changes_when_content_changes(tmp_path: Path) -> None: a = _write(tmp_path / "bzImage", b"kernel") b = _write(tmp_path / "rootdisk.img", b"rootfs") diff --git a/tests/test_vm_cli.py b/tests/test_vm_cli.py index 114c6b6..710c231 100644 --- a/tests/test_vm_cli.py +++ b/tests/test_vm_cli.py @@ -317,7 +317,9 @@ def _stub_adb_connect_wait(monkeypatch: pytest.MonkeyPatch) -> None: success so these tests are deterministic and fast regardless of adb presence. """ - monkeypatch.setattr(vm_backend.VmDeviceBackend, "_wait_for_adb_connect", lambda _self: None) + monkeypatch.setattr( + vm_backend.VmDeviceBackend, "_wait_for_adb_connect", lambda _self, *_a: None + ) @pytest.mark.usefixtures("_stub_adb_connect_wait") diff --git a/tests/test_vm_qemu.py b/tests/test_vm_qemu.py index 0adbbb8..0692ca1 100644 --- a/tests/test_vm_qemu.py +++ b/tests/test_vm_qemu.py @@ -250,6 +250,8 @@ def test_no_pidfile_not_running(self, tmp_path: Path) -> None: def test_live_pid_running(self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: (tmp_path / "qemu.pid").write_text("99") monkeypatch.setattr("beetroot.vm.qemu.os.kill", lambda *_a: None) + # Live AND the /proc cmdline names this instance's QEMU. + monkeypatch.setattr(qemu.QemuProcess, "_pid_is_qemu", lambda _self, _pid: True) assert qemu.QemuProcess(tmp_path).is_running() is True def test_stale_pid_esrch_not_running( @@ -263,17 +265,115 @@ def _kill(_pid: int, _sig: int) -> None: monkeypatch.setattr("beetroot.vm.qemu.os.kill", _kill) assert qemu.QemuProcess(tmp_path).is_running() is False - def test_eperm_counts_as_running(self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + def test_eperm_alive_but_identity_confirmed_is_running( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: (tmp_path / "qemu.pid").write_text("99") def _kill(_pid: int, _sig: int) -> None: raise OSError(errno.EPERM, "operation not permitted") monkeypatch.setattr("beetroot.vm.qemu.os.kill", _kill) + monkeypatch.setattr(qemu.QemuProcess, "_pid_is_qemu", lambda _self, _pid: True) assert qemu.QemuProcess(tmp_path).is_running() is True + def test_live_but_not_qemu_pid_not_running( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + # PID reuse: the recorded PID is live (signal-0 succeeds) but /proc says + # it is NOT this instance's QEMU — is_running must report False so the + # already-running guard and the vm.process doctor row don't false-green + # (issue #162). + (tmp_path / "qemu.pid").write_text("99") + monkeypatch.setattr("beetroot.vm.qemu.os.kill", lambda *_a: None) + monkeypatch.setattr(qemu.QemuProcess, "_pid_is_qemu", lambda _self, _pid: False) + assert qemu.QemuProcess(tmp_path).is_running() is False + + +class TestQemuProcessPidIdentity: + """The PID-identity guard that stops a reused PID being signalled (#162).""" + + def test_pid_is_qemu_matches_our_instance( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + proc = qemu.QemuProcess(tmp_path) + # A real QEMU argv carries the instance dir (kernel/rootfs live under it) + # and the qemu-system binary — both must be present to match. + cmdline = f"qemu-system-x86_64\x00-kernel\x00{tmp_path}/bzImage\x00" + monkeypatch.setattr(qemu.QemuProcess, "_read_proc_cmdline", lambda _self, _pid: cmdline) + assert proc._pid_is_qemu(123) is True + + def test_pid_is_qemu_rejects_non_qemu_process( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + proc = qemu.QemuProcess(tmp_path) + # A reused PID running something else, even if it somehow references the + # instance path, is not QEMU. + cmdline = f"/bin/sh\x00-c\x00ls {tmp_path}\x00" + monkeypatch.setattr(qemu.QemuProcess, "_read_proc_cmdline", lambda _self, _pid: cmdline) + assert proc._pid_is_qemu(123) is False + + def test_pid_is_qemu_rejects_qemu_for_other_instance( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + proc = qemu.QemuProcess(tmp_path / "alpha") + # A QEMU process, but for a DIFFERENT instance directory. + cmdline = f"qemu-system-x86_64\x00-kernel\x00{tmp_path}/bravo/bzImage\x00" + monkeypatch.setattr(qemu.QemuProcess, "_read_proc_cmdline", lambda _self, _pid: cmdline) + assert proc._pid_is_qemu(123) is False + + def test_pid_is_qemu_false_when_proc_gone( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + proc = qemu.QemuProcess(tmp_path) + monkeypatch.setattr(qemu.QemuProcess, "_read_proc_cmdline", lambda _self, _pid: None) + assert proc._pid_is_qemu(123) is False + + def test_read_proc_cmdline_reads_proc( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + proc = qemu.QemuProcess(tmp_path) + monkeypatch.setattr(Path, "read_text", lambda _self: "qemu-system-x86_64\x00") + assert proc._read_proc_cmdline(123) == "qemu-system-x86_64\x00" + + def test_read_proc_cmdline_none_on_oserror( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + proc = qemu.QemuProcess(tmp_path) + + def _boom(_self: Path) -> str: + raise OSError("no /proc") + + monkeypatch.setattr(Path, "read_text", _boom) + assert proc._read_proc_cmdline(123) is None + class TestQemuProcessStart: + def test_start_proceeds_when_pid_is_live_but_not_qemu( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + # PID reuse: a stale pidfile names a live-but-unrelated PID. start() must + # NOT falsely abort with "already running" — is_running routes through + # the identity guard, sees the PID is not our QEMU, and the launch + # proceeds (issue #162). + inst = tmp_path / "inst" + inst.mkdir() + (inst / "qemu.pid").write_text("4242") + # Live signal-0 probe, but /proc says the PID is NOT our QEMU. + monkeypatch.setattr("beetroot.vm.qemu.os.kill", lambda *_a: None) + monkeypatch.setattr(qemu.QemuProcess, "_pid_is_qemu", lambda _self, _pid: False) + + class _FakePopen: + pid = 5151 + + def __init__(self, argv: list[str], **kwargs: object) -> None: ... + + monkeypatch.setattr("beetroot.vm.qemu.subprocess.Popen", _FakePopen) + proc = qemu.QemuProcess(inst) + # No QemuLaunchError raised — the reused PID did not block the start. + assert proc.start(["qemu-system-x86_64"]) == 5151 + assert proc.read_pid() == 5151 + def test_start_writes_pidfile_and_returns_pid( self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: @@ -371,8 +471,10 @@ def test_terminate_signals_and_removes_pidfile( (tmp_path / "qemu.pid").write_text("321") sent: list[tuple[int, int]] = [] monkeypatch.setattr("beetroot.vm.qemu.os.kill", lambda pid, sig: sent.append((pid, sig))) - # Process exits promptly after SIGTERM → no SIGKILL escalation. - monkeypatch.setattr(qemu.QemuProcess, "_pid_alive", lambda _self, _pid: False) + # PID still names this instance's QEMU on the pre-signal check, then the + # process exits promptly → no SIGKILL escalation. + identity = iter([True, False]) + monkeypatch.setattr(qemu.QemuProcess, "_pid_is_qemu", lambda _self, _pid: next(identity)) assert qemu.QemuProcess(tmp_path).terminate() is True assert sent == [(321, signal.SIGTERM)] assert not (tmp_path / "qemu.pid").exists() @@ -385,7 +487,7 @@ def test_terminate_escalates_to_sigkill_when_wedged( (tmp_path / "qemu.pid").write_text("321") sent: list[tuple[int, int]] = [] monkeypatch.setattr("beetroot.vm.qemu.os.kill", lambda pid, sig: sent.append((pid, sig))) - monkeypatch.setattr(qemu.QemuProcess, "_pid_alive", lambda _self, _pid: True) + monkeypatch.setattr(qemu.QemuProcess, "_pid_is_qemu", lambda _self, _pid: True) # Collapse the grace window so the test doesn't burn real seconds. monkeypatch.setattr("beetroot.vm.qemu._TERM_GRACE_SECONDS", 0.0) monkeypatch.setattr("beetroot.vm.qemu.time.sleep", lambda _s: None) @@ -397,14 +499,14 @@ def test_terminate_escalates_to_sigkill_when_wedged( def test_terminate_polls_then_exits_before_sigkill( self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: - # Process is alive on the first poll but exits before the deadline: - # SIGTERM is sent, the poll loop sleeps once, then the process is - # gone → no SIGKILL. + # Identity holds on the pre-signal check and the first poll, then the + # process is gone: SIGTERM is sent, the poll loop sleeps once, then the + # PID no longer names our QEMU → no SIGKILL. (tmp_path / "qemu.pid").write_text("321") sent: list[tuple[int, int]] = [] monkeypatch.setattr("beetroot.vm.qemu.os.kill", lambda pid, sig: sent.append((pid, sig))) - alive = iter([True, False]) - monkeypatch.setattr(qemu.QemuProcess, "_pid_alive", lambda _self, _pid: next(alive)) + identity = iter([True, True, False]) + monkeypatch.setattr(qemu.QemuProcess, "_pid_is_qemu", lambda _self, _pid: next(identity)) slept: list[float] = [] def _sleep(s: float) -> None: @@ -419,33 +521,30 @@ def test_terminate_no_sigkill_if_process_gone_at_deadline( self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: # Grace window already elapsed (0s → loop body never runs), but the - # process has exited by the final liveness check → no SIGKILL. + # process has exited by the final identity check → no SIGKILL. (tmp_path / "qemu.pid").write_text("321") sent: list[tuple[int, int]] = [] monkeypatch.setattr("beetroot.vm.qemu.os.kill", lambda pid, sig: sent.append((pid, sig))) monkeypatch.setattr("beetroot.vm.qemu._TERM_GRACE_SECONDS", 0.0) - monkeypatch.setattr(qemu.QemuProcess, "_pid_alive", lambda _self, _pid: False) + # True for the pre-signal gate, False at the deadline check. + identity = iter([True, False]) + monkeypatch.setattr(qemu.QemuProcess, "_pid_is_qemu", lambda _self, _pid: next(identity)) assert qemu.QemuProcess(tmp_path).terminate() is True assert sent == [(321, signal.SIGTERM)] - def test_pid_alive_probes_signal_zero( + def test_terminate_refuses_live_but_not_qemu_pid( self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: - proc = qemu.QemuProcess(tmp_path) - monkeypatch.setattr("beetroot.vm.qemu.os.kill", lambda *_a: None) - assert proc._pid_alive(99) is True - - def _esrch(_pid: int, _sig: int) -> None: - raise OSError(errno.ESRCH, "no such process") - - monkeypatch.setattr("beetroot.vm.qemu.os.kill", _esrch) - assert proc._pid_alive(99) is False - - def _eperm(_pid: int, _sig: int) -> None: - raise OSError(errno.EPERM, "operation not permitted") - - monkeypatch.setattr("beetroot.vm.qemu.os.kill", _eperm) - assert proc._pid_alive(99) is True + # PID reuse: the recorded PID is live but belongs to an unrelated + # process. terminate() must NOT signal it (no SIGTERM, no SIGKILL), + # return False, and clear the stale pidfile (issue #162). + (tmp_path / "qemu.pid").write_text("321") + sent: list[tuple[int, int]] = [] + monkeypatch.setattr("beetroot.vm.qemu.os.kill", lambda pid, sig: sent.append((pid, sig))) + monkeypatch.setattr(qemu.QemuProcess, "_pid_is_qemu", lambda _self, _pid: False) + assert qemu.QemuProcess(tmp_path).terminate() is False + assert sent == [] + assert not (tmp_path / "qemu.pid").exists() def test_terminate_no_pidfile_is_noop(self, tmp_path: Path) -> None: assert qemu.QemuProcess(tmp_path).terminate() is False @@ -453,7 +552,20 @@ def test_terminate_no_pidfile_is_noop(self, tmp_path: Path) -> None: def test_terminate_dead_process_removes_pidfile( self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: + # The recorded PID is gone (its /proc entry is absent), so _pid_is_qemu + # is False, os.kill is never reached, and the stale pidfile is cleared. + (tmp_path / "qemu.pid").write_text("321") + monkeypatch.setattr(qemu.QemuProcess, "_pid_is_qemu", lambda _self, _pid: False) + assert qemu.QemuProcess(tmp_path).terminate() is False + assert not (tmp_path / "qemu.pid").exists() + + def test_terminate_sigterm_race_already_gone( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + # Identity holds at the gate but the process dies before SIGTERM lands + # (ESRCH) → signalled flips back to False, no escalation, pidfile gone. (tmp_path / "qemu.pid").write_text("321") + monkeypatch.setattr(qemu.QemuProcess, "_pid_is_qemu", lambda _self, _pid: True) def _kill(_pid: int, _sig: int) -> None: raise OSError(errno.ESRCH, "no such process") @@ -468,8 +580,9 @@ def test_terminate_tolerates_pidfile_unlink_failure( # An already-vanished pidfile (e.g. concurrent down) must not raise. (tmp_path / "qemu.pid").write_text("321") monkeypatch.setattr("beetroot.vm.qemu.os.kill", lambda *_a: None) - # Process exits promptly → skip the escalation poll loop. - monkeypatch.setattr(qemu.QemuProcess, "_pid_alive", lambda _self, _pid: False) + # True at the gate, then gone on the first escalation check → no SIGKILL. + identity = iter([True, False]) + monkeypatch.setattr(qemu.QemuProcess, "_pid_is_qemu", lambda _self, _pid: next(identity)) def _unlink(_self: Path, *_a: object, **_k: object) -> None: raise OSError("gone")