fix(cli): stop is_container() false-positive on hosts running containers - #58145
fix(cli): stop is_container() false-positive on hosts running containers#58145baleian wants to merge 1 commit into
Conversation
Duplicate of #58141 — both fix #58135 by restricting the |
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: LGTM
Small, focused fix that adds a cgroup v2 detection fallback in is_container(). Adds only 61 lines touching one file.
Looks Good
- Good fallback chain: original cgroup v1 check, then
/sys/fs/cgroup/cgroup.controllersv2 check - Returns
Falsegracefully on any parsing error (defensive) - No test changes — but this is a one-line root cause fix, not a behavioral change that needs new tests
Reviewed by Hermes Agent
tonydwb
left a comment
There was a problem hiding this comment.
Supplementary review: confirms LGTM. is_container() false-positive fix is well-scoped, 2 files. The prior COMMENT review found no blocking issues. No additional concerns.
Reviewed by Hermes Agent
|
Thanks for the focused regression fix. Current main still performs the whole-table marker scan at The root-mount check in this PR preserves the existing cgroup-v2 positive path while excluding the reported non-root The member triage comment identifies open PR #58141 as the earlier canonical implementation of the same mechanism. The patches have overlapping intent, although this PR adds an additional positive root-overlay regression. Automated hermes-sweeper review. |
What does this PR do?
hermes_constants.is_container()false-positives on a plain Linux host whenever a Docker container is running with the containerd image store (Docker Desktop's default).On cgroup v2,
/proc/1/cgroupis just0::/, sois_container()falls back to scanning/proc/self/mountinfoforkubepods/containerd/crio. But every container the host runs with the containerd snapshotter contributes an overlay mount whoselowerdir=/var/lib/containerd/...option string contains the substringcontainerd— so the whole-table scan classifies the host as being inside a container. Because the result is cached in_container_detectedper process, the answer permanently depends on whether any container happened to be running the first time each process called it.Downstream,
get_subprocess_home()under the defaultterminal.home_mode: autothen returns{HERMES_HOME}/homeinstead of the real userHOME, so every subprocess (browser worker, ACP/CLI executors, dep-ensure, …) gets an empty per-profile HOME andbrowser_navigatefails with "Chrome not found."Fix: in the mountinfo fallback, inspect only the root mount (the line whose mount point is
/). Inside a container that line is the runtime's overlay rootfs (containerd/crio paths); on a host it is a regular block device. Markers on any other mount — including thelowerdir=options of containers the host itself runs — no longer count. This also removes the time-dependence at its source (the signal now depends only on our own rootfs), so caching stays safe and suggestion #3 in the issue is unnecessary.Related Issue
Fixes #58135
Type of Change
Changes Made
hermes_constants.py—is_container(): the cgroup-v2/proc/self/mountinfofallback now matcheskubepods/containerd/crioonly on the root mount line (mount point/), instead of substring-scanning the entire mount table. Docstring updated.tests/test_hermes_constants.py— two regression tests inTestIsContainer:test_host_running_containers_not_flagged— a cgroup-v2 host whose root is a block device but which runs a containerd container (overlaylowerdir=/var/lib/containerd/...on a non-root mount) is not flagged. (Fails on the old code, passes now.)test_detects_containerd_root_overlay_lowerdir— a container whose own root/overlay carries thecontainerdmarker only in itslowerdir=options is still detected (guards against over-narrowing the fix).How to Test
Reproduction (Linux host with Docker's containerd image store, cgroup v2):
Unit tests:
All 10
TestIsContainercases pass, including the two new ones.Checklist
Code
fix(cli): …)TestIsContainer(all 10) greenDocumentation & Housekeeping
is_container()docstring to document the root-mount-only behavior/proc/self/mountinforead is Linux-only and stays inside the existingtry/except OSError; no behavior change on macOS/Windows