fix(cli): correct --hardkill-count off-by-one in worker signal handler#648
fix(cli): correct --hardkill-count off-by-one in worker signal handler#648gitcommit90 wants to merge 1 commit into
Conversation
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 taskiq-python#647
GefMar
left a comment
GefMar
left a comment
There was a problem hiding this comment.
A few notes on the signal semantics and test reliability.
| # Hard kill is a signal that we should stop | ||
| # everything immediately. | ||
| if hardkill_counter > args.hardkill_count: | ||
| if hardkill_counter >= args.hardkill_count: |
There was a problem hiding this comment.
Before changing this condition, could we clarify the contract? With this change, --hardkill-count 3 kills the worker on the fourth signal, while the docs say that sending three signals performs a hard kill. We should pick one interpretation and align the code, CLI help, docs, and test.
| ) | ||
| children = _worker_child_pids(proc.pid) | ||
| if children: | ||
| worker_pid = children[0] |
There was a problem hiding this comment.
On macOS, the first child is usually multiprocessing.resource_tracker, not the worker. It ignores SIGINT, so this test will fail there. We need a reliable way to get the actual worker PID.
There was a problem hiding this comment.
What if in _worker_child_pids we will filter out all processes that are not python?
| if worker_pid is None: | ||
| pytest.fail("No worker child process spawned in time") | ||
| # Extra settle time so the child's signal handlers are installed. | ||
| time.sleep(2.0) |
There was a problem hiding this comment.
A fixed two-second sleep does not guarantee that the worker has installed its signal handlers, especially on a busy CI runner. Could we wait for an explicit readiness signal from the test broker instead?
There was a problem hiding this comment.
We already set asyncio.event in the broker. I suppose, we can reuse it to fit our needs. Like, we only start killing, when the event is set. Reseting in on every iteration.
| ) | ||
| finally: | ||
| if proc.poll() is None: | ||
| proc.kill() |
There was a problem hiding this comment.
proc.kill() only stops the supervisor, so the hanging worker may be left running after a test failure. It would be safer to terminate the whole process group or explicitly clean up all child processes.
3 participants
Summary
Fixes #647:
--hardkill-count Nwas off by one in the worker signal handler.Docs/CLI help describe N as the number of termination signals allowed before a hard kill (hard kill on signal N+1). The handler compared
hardkill_counter > args.hardkill_countbefore incrementing, so with the defaultN=3the hard kill fired on the 5th signal instead of the 4th.Changes
taskiq/cli/worker/run.py: change>to>=so hard kill runs on signal N+1.tests/cli/worker/hanging_broker.pyandtests/cli/worker/test_hardkill_count.py— integration test that spawns a real worker with a broker whose graceful shutdown hangs, sends SIGINTs to the worker child, and asserts survival through N signals and hard kill on N+1.Test plan
pytest tests/ -q -n auto→ 298 passedruff check,ruff format --check,black --check,mypyclean on touched filesCloses #647