Skip to content

Commit 418afd1

Browse files
committed
fix(cli): correct --hardkill-count off-by-one in worker signal handler
The worker's interrupt handler compared hardkill_counter to args.hardkill_count with '>' before incrementing, so N allowed termination signals actually required N+2 signals to hard-kill (e.g. the default --hardkill-count 3 hard-killed on the 5th signal). Use '>=' so signal N+1 performs the hard kill, matching the CLI help text: 'Number of termination signals to the main process before performing a hardkill.' Add an integration test that spawns a real worker with a broker whose graceful shutdown hangs, sends SIGINTs to the worker child process, and asserts the worker survives N signals and is hard-killed on signal N+1. The test fails on the previous comparison and passes with this fix. Fixes #647
1 parent ae2b788 commit 418afd1

3 files changed

Lines changed: 155 additions & 1 deletion

File tree

taskiq/cli/worker/run.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,7 @@ def interrupt_handler(signum: int, _frame: Any) -> None:
110110
shutdown_event.set()
111111
# Hard kill is a signal that we should stop
112112
# everything immediately.
113-
if hardkill_counter > args.hardkill_count:
113+
if hardkill_counter >= args.hardkill_count:
114114
logger.warning("Hard kill. Exiting.")
115115
raise KeyboardInterrupt
116116
hardkill_counter += 1

tests/cli/worker/hanging_broker.py

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
"""Broker module used by test_hardkill_count subprocess tests.
2+
3+
The broker never yields messages and its graceful shutdown blocks on an
4+
event that is never set, so a running worker can only exit via the
5+
hard-kill path (KeyboardInterrupt raised from the signal handler).
6+
"""
7+
8+
import asyncio
9+
from collections.abc import AsyncGenerator
10+
11+
from taskiq.abc.broker import AsyncBroker
12+
from taskiq.message import BrokerMessage
13+
14+
15+
class HangingBroker(AsyncBroker):
16+
"""Broker that idles forever and never finishes graceful shutdown."""
17+
18+
async def kick(self, message: BrokerMessage) -> None:
19+
"""No-op kick."""
20+
21+
def listen(self) -> AsyncGenerator[bytes, None]:
22+
"""Return an async generator that never yields."""
23+
24+
async def _gen() -> AsyncGenerator[bytes, None]:
25+
await asyncio.Event().wait()
26+
yield b"" # pragma: no cover
27+
28+
return _gen()
29+
30+
async def shutdown(self) -> None:
31+
"""Block forever, simulating a graceful shutdown that hangs."""
32+
await asyncio.Event().wait()
33+
34+
35+
broker = HangingBroker()
Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,119 @@
1+
"""Integration test for --hardkill-count signal semantics.
2+
3+
Spawns a real worker subprocess whose broker never finishes graceful
4+
shutdown, sends SIGINTs directly to a worker child process (the process
5+
that installs the hard-kill counting signal handler), and checks how
6+
many signals it takes to hard-kill it.
7+
8+
Documented behavior (``--hardkill-count N``): N termination signals are
9+
allowed before performing a hardkill, i.e. signals 1..N only set the
10+
shutdown event and signal N+1 raises KeyboardInterrupt in the worker.
11+
"""
12+
13+
import os
14+
import signal
15+
import subprocess
16+
import sys
17+
import time
18+
from pathlib import Path
19+
20+
import psutil
21+
import pytest
22+
23+
REPO_ROOT = Path(__file__).resolve().parents[3]
24+
SIGNAL_SETTLE_SECONDS = 0.6
25+
WAIT_TIMEOUT = 15.0
26+
27+
28+
def _pid_alive(pid: int) -> bool:
29+
"""Check whether a process exists."""
30+
try:
31+
os.kill(pid, 0)
32+
except OSError:
33+
return False
34+
return True
35+
36+
37+
def _worker_child_pids(main_pid: int) -> list[int]:
38+
"""Return pids of worker child processes of the main process."""
39+
try:
40+
parent = psutil.Process(main_pid)
41+
except psutil.NoSuchProcess:
42+
return []
43+
return [child.pid for child in parent.children(recursive=False)]
44+
45+
46+
@pytest.mark.skipif(
47+
sys.platform == "win32",
48+
reason="SIGINT hard-kill semantics differ on Windows",
49+
)
50+
def test_hardkill_count_signals_before_hardkill() -> None:
51+
"""N signals are graceful; signal N+1 hard-kills the worker process."""
52+
hardkill_count = 3
53+
env = os.environ.copy()
54+
env["PYTHONPATH"] = str(REPO_ROOT) + os.pathsep + env.get("PYTHONPATH", "")
55+
proc = subprocess.Popen( # noqa: S603
56+
[
57+
sys.executable,
58+
"-m",
59+
"taskiq",
60+
"worker",
61+
"tests.cli.worker.hanging_broker:broker",
62+
"--hardkill-count",
63+
str(hardkill_count),
64+
"--workers",
65+
"1",
66+
"--no-configure-logging",
67+
# Keep graceful shutdown pending so only the hard-kill
68+
# path can terminate the worker during the test.
69+
"--shutdown-timeout",
70+
"3600",
71+
],
72+
cwd=REPO_ROOT,
73+
env=env,
74+
stdout=subprocess.DEVNULL,
75+
stderr=subprocess.DEVNULL,
76+
)
77+
try:
78+
# Wait for the worker child to spawn and install its handlers.
79+
deadline = time.monotonic() + WAIT_TIMEOUT
80+
worker_pid: int | None = None
81+
while time.monotonic() < deadline:
82+
if proc.poll() is not None:
83+
pytest.fail(
84+
f"Worker exited during startup: returncode={proc.returncode}",
85+
)
86+
children = _worker_child_pids(proc.pid)
87+
if children:
88+
worker_pid = children[0]
89+
break
90+
time.sleep(0.25)
91+
if worker_pid is None:
92+
pytest.fail("No worker child process spawned in time")
93+
# Extra settle time so the child's signal handlers are installed.
94+
time.sleep(2.0)
95+
96+
# Signals 1..N: only set the shutdown event; the broker's graceful
97+
# shutdown hangs, so the worker process must still be alive.
98+
for signal_number in range(1, hardkill_count + 1):
99+
os.kill(worker_pid, signal.SIGINT)
100+
time.sleep(SIGNAL_SETTLE_SECONDS)
101+
assert _pid_alive(worker_pid), (
102+
f"Worker exited on signal {signal_number} of "
103+
f"{hardkill_count} allowed graceful signals"
104+
)
105+
106+
# Signal N+1: hard kill raises KeyboardInterrupt in the worker.
107+
os.kill(worker_pid, signal.SIGINT)
108+
deadline = time.monotonic() + WAIT_TIMEOUT
109+
while time.monotonic() < deadline and _pid_alive(worker_pid):
110+
time.sleep(0.1)
111+
assert not _pid_alive(worker_pid), (
112+
f"Worker still alive after {hardkill_count + 1} signals; "
113+
f"--hardkill-count {hardkill_count} should hard-kill on "
114+
f"signal {hardkill_count + 1}"
115+
)
116+
finally:
117+
if proc.poll() is None:
118+
proc.kill()
119+
proc.wait(timeout=10)

0 commit comments

Comments
 (0)