Repository navigation
fix(eventing): reclaim a pidfile that names our own PID - #901
Conversation
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 <mrsabath@gmail.com>
aslom
left a comment
aslom
left a comment
There was a problem hiding this comment.
Summary
The diagnosis is right and the fix is the minimal correct one. A pidfile naming our own PID
cannot describe another live process, because __enter__ has not written the file yet — so
reclaiming it is sound, and it closes the PID 1 crash-loop in #889. I checked the premise
rather than taking it on trust: both images use exec-form CMD ["python", "-m", ...] with no
ENTRYPOINT wrapper, so the daemon really is PID 1, and both set TMPDIR=/data onto the
mounted volume.
I also went looking for the obvious way this fix could be wrong — two processes that are each
PID 1 in their own namespace sharing one volume, which would make own-PID equality ambiguous
and silently disarm the guard. It cannot happen here: the demo overlay's JSON 6902 patch
attaches the PVC to eventbridge only, and eventbridge is replicas: 1 with
strategy: Recreate specifically to avoid two pods sharing a volume; the KEDA-scaled
eventrunner keeps its per-pod emptyDir. Worth keeping in mind if eventrunner is ever given a
shared volume, since that is the assumption the elif now rests on.
Scoping out the boot-id / start-time work is the right call — it changes the pidfile format and
the __exit__ comparison, and the crash-loop does not need it.
Comments below are all non-blocking: one docstring wording nit and two notes on the test
substitution. Nothing here needs to hold up the merge.
Author: mrsabath (MEMBER — maintainer)
Areas reviewed: Python (eventing/shared/pidfile.py), tests, plus Dockerfiles and k8s/
manifests read as context to verify the PID 1 and volume-sharing claims
Agent/IDE config (.claude/.vscode): none
Commits: 1 commit, all signed-off: yes (DCO check passing)
CI status: passing — all 12 checks green, including eventing-test, lint, codeql, trivy-scan
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 <mrsabath@gmail.com>
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 <mrsabath@gmail.com>
6ddcc6d to
fdf3aec
Compare
3 participants
What
A pidfile naming our own PID is treated as stale and reclaimed, instead of being read
as "another process is already running".
Fixes #889.
Why
TMPDIR=/datain both images, so the pidfile is on the mounted volume. An abrupt exit(OOMKill at the
512Milimit,SIGKILL, node failure) leaves it containing1. Onrestart the process is PID 1 again,
os.kill(1, 0)succeeds because PID 1 is thecaller itself, and
__enter__raisesSystemExit: already running as pid 1— on thebase, kind and test manifests until the pod is deleted, and on the demo overlay's PVC
past that too.
Verified:
_pid_alive(1)returnsTruefrom an ordinary process (EPERM — "exists butwe don't own it"), and in a container it succeeds outright. Either way the guard
misfires.
Collateral, called out
Two existing tests used our own PID as a stand-in for "a live process", which this
change deliberately makes reclaimable. They now use
os.getppid()— alive, and not us— so the real liveness guard is still covered and still refuses.
Not in scope
The issue also suggests recording a boot id or process start time, to catch general PID
reuse rather than just this case. That changes the pidfile format and the
__exit__comparison, so it is left out; this PR fixes the crash-loop only.
Verification
python -m pytest tests/test_pidfile.py— 7 passedeventing/suite on this base — 898 passed, 7 skippedruff checkcleanrefuses with
already running as pid <ours>; the new one reclaims itfrom
_pid_alivesemantics plus theTMPDIR=/datamanifests, not observed in a pod