From f0320f867989c9926054aaa08ac47db9f7db4b93 Mon Sep 17 00:00:00 2001 From: Mariusz Sabath Date: Tue, 6 Oct 2026 23:13:54 -0400 Subject: [PATCH 1/2] fix(eventing): reclaim a pidfile that names our own PID After an abrupt exit, EventBridge and EventRunner could refuse to start with `already running as pid 1` and crash-loop until the pod was deleted. Both images set `TMPDIR=/data`, so the pidfile lives on the mounted volume rather than in ephemeral container storage. An OOMKill at the 512Mi limit, a SIGKILL or a node failure leaves it behind containing `1`. On restart the process is PID 1 again, so `os.kill(1, 0)` succeeds -- PID 1 is the caller itself -- and the liveness check concludes the previous process is still running. An emptyDir survives a container restart, and on the demo overlay's PVC the pidfile outlives the pod too, so deleting the pod was not enough. A pidfile naming our own PID cannot be another live process, because we have not written the file yet. Treat it as stale and reclaim it. `test_live_pidfile_refuses_start` and `test_run_force_overrides` used our own PID to stand in for "a live process", which this change deliberately makes reclaimable, so they now use `os.getppid()` -- alive, and not us. The real liveness guard is unchanged and still refuses. Reported by @huang195 during review of rossoctl/rossoctl#2609. Fixes #889 Assisted-By: Claude Code Signed-off-by: Mariusz Sabath --- eventing/shared/pidfile.py | 14 +++++++++++--- eventing/tests/test_pidfile.py | 30 ++++++++++++++++++++++++++---- 2 files changed, 37 insertions(+), 7 deletions(-) diff --git a/eventing/shared/pidfile.py b/eventing/shared/pidfile.py index 9b6dc949..0de68812 100644 --- a/eventing/shared/pidfile.py +++ b/eventing/shared/pidfile.py @@ -2,8 +2,8 @@ `PidFile(name)` writes `${TMPDIR}/rossoctl-keda1/.pid` on __enter__, removes it on __exit__. Refuses to start if the file exists and points at a live process -(unless RUN_FORCE=1 is set in the env). Stale PID files (process gone) are -silently reclaimed. +(unless RUN_FORCE=1 is set in the env). Stale PID files (process gone, or naming +our own PID) are silently reclaimed. """ from __future__ import annotations @@ -44,7 +44,15 @@ def __enter__(self) -> "PidFile": existing = int(self._path.read_text().strip() or "0") except ValueError: existing = 0 - if existing and _pid_alive(existing): + if existing == os.getpid(): + # We have not written this file yet, so our own PID in it means the + # previous process died abruptly and the restarted one was handed the + # same PID. In a container that is every restart: the process is PID 1, + # `_pid_alive(1)` is True because PID 1 is the caller itself, and the + # guard below would refuse to start forever ("already running as pid 1"). + print(f"[{self._name}] reclaimed stale pidfile naming our own pid" + f" {existing}: {self._path}") + elif existing and _pid_alive(existing): if os.environ.get("RUN_FORCE") != "1": raise SystemExit( f"[{self._name}] already running as pid {existing}" diff --git a/eventing/tests/test_pidfile.py b/eventing/tests/test_pidfile.py index f526a5b9..6fb22f15 100644 --- a/eventing/tests/test_pidfile.py +++ b/eventing/tests/test_pidfile.py @@ -22,24 +22,46 @@ def test_stale_pidfile_is_reclaimed(tmp_path: pathlib.Path): def test_live_pidfile_refuses_start(tmp_path: pathlib.Path, monkeypatch): - # Use *our own* pid — guaranteed to be alive - (tmp_path / "eb-test.pid").write_text(f"{os.getpid()}\n") + # Our parent: alive, and -- unlike our own pid -- not us. A pidfile naming our own + # pid is deliberately reclaimable (see the PID 1 test below), so it cannot stand in + # for "another process is running" here. + (tmp_path / "eb-test.pid").write_text(f"{os.getppid()}\n") monkeypatch.delenv("RUN_FORCE", raising=False) with pytest.raises(SystemExit) as exc: with PidFile("eb-test", directory=tmp_path): pass assert "already running" in str(exc.value) # Pidfile must remain untouched so the running process's cleanup still works - assert (tmp_path / "eb-test.pid").read_text().strip() == str(os.getpid()) + assert (tmp_path / "eb-test.pid").read_text().strip() == str(os.getppid()) def test_run_force_overrides(tmp_path: pathlib.Path, monkeypatch): - (tmp_path / "eb-test.pid").write_text(f"{os.getpid()}\n") + (tmp_path / "eb-test.pid").write_text(f"{os.getppid()}\n") monkeypatch.setenv("RUN_FORCE", "1") with PidFile("eb-test", directory=tmp_path) as pf: assert pf.path.read_text().strip() == str(os.getpid()) +def test_a_pidfile_naming_our_own_pid_is_reclaimed_not_refused( + tmp_path: pathlib.Path, monkeypatch): + """The container crash-loop: in a pod the restarted process is PID 1 again. + + An abrupt exit (OOMKill at the 512Mi limit, SIGKILL, node failure) leaves a pidfile + naming 1 on the mounted volume -- both images set TMPDIR=/data -- and on restart + `_pid_alive(1)` is True because PID 1 is the caller itself. The guard then refused + to start forever, and on the demo overlay's PVC deleting the pod did not help. + + Our own PID stands in for PID 1: the property that matters is that the pidfile names + the process doing the check, which cannot be a different live process. + Reported by @huang195 during review of rossoctl/rossoctl#2609. + """ + (tmp_path / "eb-test.pid").write_text(f"{os.getpid()}\n") + monkeypatch.delenv("RUN_FORCE", raising=False) + with PidFile("eb-test", directory=tmp_path) as pf: + assert pf.path.read_text().strip() == str(os.getpid()) + assert not pf.path.exists(), "and normal cleanup still applies afterwards" + + def test_pid_alive_zero_and_negative_are_dead(): assert not _pid_alive(0) assert not _pid_alive(-1) From fdf3aecd241f3ae42e90bbb752812bb3ae15bf43 Mon Sep 17 00:00:00 2001 From: Mariusz Sabath Date: Wed, 7 Oct 2026 10:31:35 -0400 Subject: [PATCH 2/2] test(eventing): address review on the pidfile PR Alek's review on #901 (approved, three non-blocking comments), all taken. The docstring said stale pidfiles are "silently reclaimed", but both reclaim paths print a line. Re-worded to "reclaimed, with a line on stdout saying so" -- it was inaccurate before this PR, and the sentence was being edited anyway. The two guard tests used `os.getppid()` as their live other process. That holds on the CI runner (`ci.yaml` runs `python -m pytest` directly on `ubuntu-latest`, so the parent is a live shell), but it degrades to 0 in exactly the environment this PR is about: run the suite as PID 1 in a container (`docker run ... pytest`) and the pidfile would hold 0, which `__enter__` reads as falsy, taking the stale branch instead of refusing. The tests would then fail with DID NOT RAISE rather than reporting anything about the guard. Both now use a `live_other_pid` fixture that spawns a sleeping child -- alive, not us, and non-zero in any PID namespace -- and terminates it in the fixture teardown. `test_run_force_overrides` asserted only that the pidfile ends up holding our own PID, which every branch of `__enter__` produces. It now asserts the override branch's WARNING line, which the stale-reclaim branch does not print, so it can fail for the right reason. Verified both ways: the stale path prints no warning, the RUN_FORCE path prints it. `tests/test_pidfile.py` -- 7 passed. `ruff check` and `ruff format --check` clean from the repository root, as CI runs them. The other 19 local failures in `test_consume_phase1.py` reproduce on the parent commit under this sandbox's Python 3.13 (the package requires >=3.14); CI's eventing-test on 3.14 is green on both the parent and this commit. Assisted-By: Claude Code Signed-off-by: Mariusz Sabath --- eventing/shared/pidfile.py | 2 +- eventing/tests/test_pidfile.py | 43 ++++++++++++++++++++++++++-------- 2 files changed, 34 insertions(+), 11 deletions(-) diff --git a/eventing/shared/pidfile.py b/eventing/shared/pidfile.py index 0de68812..796c26ce 100644 --- a/eventing/shared/pidfile.py +++ b/eventing/shared/pidfile.py @@ -3,7 +3,7 @@ `PidFile(name)` writes `${TMPDIR}/rossoctl-keda1/.pid` on __enter__, removes it on __exit__. Refuses to start if the file exists and points at a live process (unless RUN_FORCE=1 is set in the env). Stale PID files (process gone, or naming -our own PID) are silently reclaimed. +our own PID) are reclaimed, with a line on stdout saying so. """ from __future__ import annotations diff --git a/eventing/tests/test_pidfile.py b/eventing/tests/test_pidfile.py index 6fb22f15..0b2a8f34 100644 --- a/eventing/tests/test_pidfile.py +++ b/eventing/tests/test_pidfile.py @@ -1,12 +1,31 @@ """PidFile: writes on enter, removes on exit, guards against double-start.""" import os import pathlib +import subprocess +import sys import pytest from shared.pidfile import PidFile, _pid_alive +@pytest.fixture +def live_other_pid(): + """A process that is alive, is not us, and has a non-zero pid in any namespace. + + `os.getppid()` was the first stand-in, but it degrades to 0 when the suite itself + runs as PID 1 in a container (`docker run ... pytest`): the pidfile would then hold + 0, `__enter__` reads a falsy `existing`, takes the stale branch and never refuses. + A spawned child has no ambient-process-tree dependency. + """ + p = subprocess.Popen([sys.executable, "-c", "import time; time.sleep(30)"]) + try: + yield p.pid + finally: + p.terminate() + p.wait() + + def test_writes_and_removes(tmp_path: pathlib.Path): with PidFile("eb-test", directory=tmp_path) as pf: assert pf.path.exists() @@ -21,25 +40,29 @@ def test_stale_pidfile_is_reclaimed(tmp_path: pathlib.Path): assert pf.path.read_text().strip() == str(os.getpid()) -def test_live_pidfile_refuses_start(tmp_path: pathlib.Path, monkeypatch): - # Our parent: alive, and -- unlike our own pid -- not us. A pidfile naming our own - # pid is deliberately reclaimable (see the PID 1 test below), so it cannot stand in - # for "another process is running" here. - (tmp_path / "eb-test.pid").write_text(f"{os.getppid()}\n") +def test_live_pidfile_refuses_start(tmp_path: pathlib.Path, monkeypatch, live_other_pid): + # The fixture's child: alive, and -- unlike our own pid -- not us. A pidfile naming + # our own pid is deliberately reclaimable (see the PID 1 test below), so it cannot + # stand in for "another process is running" here. + (tmp_path / "eb-test.pid").write_text(f"{live_other_pid}\n") monkeypatch.delenv("RUN_FORCE", raising=False) with pytest.raises(SystemExit) as exc: with PidFile("eb-test", directory=tmp_path): pass assert "already running" in str(exc.value) # Pidfile must remain untouched so the running process's cleanup still works - assert (tmp_path / "eb-test.pid").read_text().strip() == str(os.getppid()) + assert (tmp_path / "eb-test.pid").read_text().strip() == str(live_other_pid) -def test_run_force_overrides(tmp_path: pathlib.Path, monkeypatch): - (tmp_path / "eb-test.pid").write_text(f"{os.getppid()}\n") +def test_run_force_overrides(tmp_path: pathlib.Path, monkeypatch, live_other_pid, capsys): + (tmp_path / "eb-test.pid").write_text(f"{live_other_pid}\n") monkeypatch.setenv("RUN_FORCE", "1") - with PidFile("eb-test", directory=tmp_path) as pf: - assert pf.path.read_text().strip() == str(os.getpid()) + with PidFile("eb-test", directory=tmp_path): + pass + # The override branch announces itself; the stale-reclaim branch does not. Without + # this, the pidfile content alone cannot tell the two apart (both end in write_text). + out = capsys.readouterr().out + assert "WARNING: overwriting live pidfile" in out def test_a_pidfile_naming_our_own_pid_is_reclaimed_not_refused(