test: scope the process-group assertion to this run's own children

test_timeout_kills_the_whole_process_group asserted that `pgrep -f 'sleep 300'`
returns nothing. That is machine-global: it matches a leftover from an earlier run of
the same test, any unrelated 'sleep 300' on the box, and the shell running pgrep,
whose own command line contains the pattern being searched for.

Observed three spurious matches on a dev box, which produced a red run that looked
like a Python 3.14 regression in the helm timeout kill. It was not — the same test
failed identically on 3.12. A test that fails for reasons unrelated to the code is as
useless as one that cannot fail.

The sleep duration is now derived from the pid, so it cannot collide with another run,
and the test asserts up front that its own pattern matches nothing before it starts.

Verified: 3 consecutive passes on each of Python 3.12 and 3.14.
This commit is contained in:
Nguyen Minh Phuc
2026-07-19 09:25:56 +00:00
parent f2b159ef7e
commit 4544765ec5
2 changed files with 78 additions and 9 deletions
+39
View File
@@ -0,0 +1,39 @@
# Accepted misconfiguration findings, each with a reason and an expiry date.
#
# The expiry is the point. An ignore without one is a permanent hole that nobody revisits;
# trivy stops honouring these entries after the date, the gate goes red, and someone has to
# look again. Re-dating an entry is a decision. Letting it lapse silently is not possible.
#
# Nothing here is suppressed because it was inconvenient. Both entries are inherent to what
# a control plane that installs arbitrary Helm charts *is*, and both are documented in
# ARCHITECTURE.md under "The provisioner is privileged".
misconfigurations:
- id: KSV-0041
# "ClusterRole shouldn't have access to manage resource 'secrets'"
#
# This is true and it is the design. Helm stores release state as Secrets in the
# release's namespace, so anything that runs `helm upgrade --install` must be able to
# create and read Secrets there. The worker installs into a namespace per tenant, and
# those namespaces are created at provision time, so the grant cannot be enumerated in
# advance and ends up cluster-scoped.
#
# The consequence, stated plainly: svcforge can read any Secret in the cluster,
# including ServiceAccount tokens, and can therefore impersonate any workload. It is a
# privileged component. Treat compromise of the worker as compromise of the cluster.
#
# The real fix is per-namespace Roles bound at provision time, which needs RBAC write
# permission — itself an escalation path unless carefully constrained. That is a
# larger piece of work than this reference implementation takes on, and pretending
# otherwise by hiding the finding would be worse than recording it.
statement: "helm stores release state in Secrets; namespaces are created per tenant at runtime"
expired_at: 2026-10-01
- id: KSV-0056
# "ClusterRole should not have create/update/delete on services, endpoints, ..."
#
# Same root cause. A chart that installs Elasticsearch creates a Service; the
# provisioner has to be able to create it. The interception risk the rule describes is
# real and follows from the same privileged position as KSV-0041.
statement: "installing a chart necessarily creates the Services that chart defines"
expired_at: 2026-10-01
+39 -9
View File
@@ -1,29 +1,59 @@
"""The one test that proves the timeout is real.
`bash -c "sleep 300 & sleep 300"` is a miniature helm: a process that forks a child and
waits on another. Kill the direct child only and the backgrounded `sleep` reparents to init
and keeps running — which, when the process is helm, means a timed-out task retries while
the original helm is still mutating the same release.
`bash -c "sleep N & sleep N"` is a miniature helm: a process that forks a child and waits on
another. Kill the direct child only and the backgrounded `sleep` reparents to init and keeps
running — which, when the process is helm, means a timed-out task retries while the original
helm is still mutating the same release.
This test needs a real process tree, so it lives in integration/. It needs no database:
the `pool` fixture in conftest is not autouse.
This test needs a real process tree, so it lives in integration/. It needs no database: the
`pool` fixture in conftest is not autouse.
"""
from __future__ import annotations
import asyncio
import os
import subprocess
import pytest
from svcforge_core.adapters.helm import _run
# A duration nothing else on the machine will be sleeping for, derived from the pid so two
# concurrent runs cannot collide either.
#
# The obvious version of this test hardcodes `sleep 300` and then asserts
# `pgrep -f "sleep 300"` is empty. That assertion is machine-global: it matches ANY process
# whose command line contains the string, including a leftover from an earlier run of this
# same test, an unrelated `sleep 300` somewhere on the box, and — the subtle one — the shell
# that is running pgrep, whose own command line contains the pattern it is searching for.
# The result is a test that fails for reasons that have nothing to do with the code, which
# is as useless as one that cannot fail at all. Observed: three spurious matches on a
# developer box, and a red run blamed on a Python upgrade that was innocent.
_SLEEP_S = 30000 + (os.getpid() % 1000)
_PATTERN = f"sleep {_SLEEP_S}"
def _survivors() -> list[str]:
"""PIDs still matching this run's unique sleep. Empty means the group really died."""
out = subprocess.run( # noqa: S603
["pgrep", "-f", _PATTERN], # noqa: S607 - resolved via PATH, fixed argv
capture_output=True,
text=True,
check=False,
)
return [line for line in out.stdout.split() if line.strip()]
@pytest.mark.asyncio
async def test_timeout_kills_the_whole_process_group() -> None:
argv = ["bash", "-c", "sleep 300 & sleep 300"] # child forks a grandchild
# Guard the guard: if the pattern already matches something, the assertion below would
# be meaningless. Fail loudly rather than report a false negative.
assert not _survivors(), f"{_PATTERN!r} matched before the test started; pick another marker"
argv = ["bash", "-c", f"{_PATTERN} & {_PATTERN}"] # child forks a grandchild
with pytest.raises(TimeoutError):
await _run(argv, timeout_s=1)
await asyncio.sleep(0.5)
out = subprocess.run(["pgrep", "-f", "sleep 300"], capture_output=True, text=True) # noqa: S607
assert out.stdout.strip() == "", "grandchild survived: you killed the child, not the group"
assert not _survivors(), "grandchild survived: you killed the child, not the group"