mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-05-29 06:31:32 +00:00
test(dockerfile): recognize s6-overlay/init as a valid PID-1; harden against historical-comment masquerade
PR #30136 CI: test_dockerfile_entrypoint_routes_through_the_init failed because the test hardcoded known_inits = ('tini', 'dumb-init', 'catatonit'). The PR replaced tini with s6-overlay's /init (which execs s6-svscan as PID 1) — same SIGCHLD-reaping contract, different name, so the substring scan against ENTRYPOINT missed it. Two-part fix: 1. Extend the accepted token list to include 's6-overlay', 's6-svscan', and '/init'. The contract these tests enforce is behavioural ('some PID-1 init reaps SIGCHLD'), so the names list is purely a recognition table and any reaper-capable family should qualify. 2. Harden test_dockerfile_installs_an_init_for_zombie_reaping (the sibling check) against comment-only matches. It was scanning the full Dockerfile text and only passed because the word 'tini' is still in a historical comment explaining why we used to use it. The next person to clean up that comment would have silently broken the test. New _instruction_text() helper joins only the parsed, non-comment Dockerfile instructions so stale comments can't satisfy the check.
This commit is contained in:
parent
472be1247d
commit
ffc1bb6393
1 changed files with 49 additions and 14 deletions
|
|
@ -5,11 +5,17 @@ they deliberately avoid snapshotting specific package versions, line numbers,
|
|||
or exact flag choices. What they DO assert is that the Dockerfile maintains
|
||||
the properties required for correct production behaviour:
|
||||
|
||||
- A PID-1 init (tini) is installed and wraps the entrypoint, so that orphaned
|
||||
- A PID-1 init is installed and wraps the entrypoint, so that orphaned
|
||||
subprocesses (MCP stdio servers, git, bun, browser daemons) get reaped
|
||||
instead of accumulating as zombies (#15012).
|
||||
- Signal forwarding runs through the init so ``docker stop`` triggers
|
||||
hermes's own graceful-shutdown path.
|
||||
|
||||
The init can be any reaper-capable PID-1: the historical lineage was
|
||||
``tini``; the current image uses s6-overlay's ``/init`` (which execs
|
||||
``s6-svscan`` as PID 1, with the same SIGCHLD-reaping property). The
|
||||
checks below accept either family — the contract is behavioural, not
|
||||
nominal.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
|
@ -24,6 +30,21 @@ DOCKERFILE = REPO_ROOT / "Dockerfile"
|
|||
DOCKERIGNORE = REPO_ROOT / ".dockerignore"
|
||||
|
||||
|
||||
# Init-process families this repo accepts as PID 1. ``tini`` /
|
||||
# ``dumb-init`` / ``catatonit`` are classic minimal reapers; s6-overlay
|
||||
# ships ``/init`` which execs ``s6-svscan`` as PID 1 (same reaper
|
||||
# contract, plus supervision of declared services). Either family
|
||||
# satisfies the zombie-reaping invariant — see issue #15012.
|
||||
_KNOWN_INIT_TOKENS: tuple[str, ...] = (
|
||||
"tini",
|
||||
"dumb-init",
|
||||
"catatonit",
|
||||
"s6-overlay",
|
||||
"s6-svscan",
|
||||
"/init",
|
||||
)
|
||||
|
||||
|
||||
@pytest.fixture(scope="module")
|
||||
def dockerfile_text() -> str:
|
||||
if not DOCKERFILE.exists():
|
||||
|
|
@ -57,8 +78,17 @@ def _run_steps(dockerfile_text: str) -> list[str]:
|
|||
]
|
||||
|
||||
|
||||
def _instruction_text(dockerfile_text: str) -> str:
|
||||
"""Join every non-comment Dockerfile instruction into one searchable
|
||||
string. Crucially excludes comments — otherwise the historical
|
||||
explanation of "we used to use tini" would silently satisfy a
|
||||
substring check long after tini was removed from the build.
|
||||
"""
|
||||
return "\n".join(_dockerfile_instructions(dockerfile_text))
|
||||
|
||||
|
||||
def test_dockerfile_installs_an_init_for_zombie_reaping(dockerfile_text):
|
||||
"""Some init (tini, dumb-init, catatonit) must be installed.
|
||||
"""Some init (tini, dumb-init, catatonit, s6-overlay) must be installed.
|
||||
|
||||
Without a PID-1 init that handles SIGCHLD, hermes accumulates zombie
|
||||
processes from MCP stdio subprocesses, git operations, browser
|
||||
|
|
@ -67,12 +97,17 @@ def test_dockerfile_installs_an_init_for_zombie_reaping(dockerfile_text):
|
|||
"""
|
||||
# Accept any of the common reapers. The contract is behavioural:
|
||||
# something must be installed that reaps orphans.
|
||||
known_inits = ("tini", "dumb-init", "catatonit")
|
||||
installed = any(name in dockerfile_text for name in known_inits)
|
||||
#
|
||||
# Scan instructions only (no comments) so a stale historical mention
|
||||
# in a comment can't masquerade as a current install. Without this,
|
||||
# removing tini from the actual build but leaving the word in a
|
||||
# comment would silently keep the test green.
|
||||
instructions = _instruction_text(dockerfile_text)
|
||||
installed = any(name in instructions for name in _KNOWN_INIT_TOKENS)
|
||||
assert installed, (
|
||||
"No PID-1 init detected in Dockerfile (looked for: "
|
||||
f"{', '.join(known_inits)}). Without an init process to reap "
|
||||
"orphaned subprocesses, hermes accumulates zombies in Docker "
|
||||
"No PID-1 init detected in Dockerfile instructions (looked for: "
|
||||
f"{', '.join(_KNOWN_INIT_TOKENS)}). Without an init process to "
|
||||
"reap orphaned subprocesses, hermes accumulates zombies in Docker "
|
||||
"deployments. See issue #15012."
|
||||
)
|
||||
|
||||
|
|
@ -80,8 +115,8 @@ def test_dockerfile_installs_an_init_for_zombie_reaping(dockerfile_text):
|
|||
def test_dockerfile_entrypoint_routes_through_the_init(dockerfile_text):
|
||||
"""The ENTRYPOINT must invoke the init, not the entrypoint script directly.
|
||||
|
||||
Installing tini is only half the fix — the container must actually run
|
||||
with tini as PID 1. If the ENTRYPOINT executes the shell script
|
||||
Installing the init is only half the fix — the container must actually
|
||||
run with it as PID 1. If the ENTRYPOINT executes the shell script
|
||||
directly, the shell becomes PID 1 and will ``exec`` into hermes,
|
||||
which then runs as PID 1 without any zombie reaping.
|
||||
"""
|
||||
|
|
@ -96,12 +131,12 @@ def test_dockerfile_entrypoint_routes_through_the_init(dockerfile_text):
|
|||
|
||||
assert entrypoint_line is not None, "Dockerfile is missing an ENTRYPOINT directive"
|
||||
|
||||
known_inits = ("tini", "dumb-init", "catatonit")
|
||||
routes_through_init = any(name in entrypoint_line for name in known_inits)
|
||||
routes_through_init = any(name in entrypoint_line for name in _KNOWN_INIT_TOKENS)
|
||||
assert routes_through_init, (
|
||||
f"ENTRYPOINT does not route through an init: {entrypoint_line!r}. "
|
||||
"If tini is only installed but not wired into ENTRYPOINT, hermes "
|
||||
"still runs as PID 1 and zombies will accumulate (#15012)."
|
||||
f"ENTRYPOINT does not route through a PID-1 init: {entrypoint_line!r}. "
|
||||
f"Expected one of {_KNOWN_INIT_TOKENS}. If the init is installed but "
|
||||
"not wired into ENTRYPOINT, hermes still runs as PID 1 and zombies "
|
||||
"will accumulate (#15012)."
|
||||
)
|
||||
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue