Skip to content

Fix airflow celery stop crashing on stale PID file - #72817

Merged
dheerajturaga merged 2 commits into
apache:mainfrom
Eason09053360:fix-celery-stop-stale-pid-file
Sep 15, 2026
Merged

dheerajturaga merged 2 commits into
apache:mainfrom
Eason09053360:fix-celery-stop-stale-pid-file

Conversation

@Eason09053360

Copy link
Copy Markdown
Contributor

When a Celery worker dies without cleaning up its PID file (SIGKILL, OOM, host reboot), airflow celery stop reads the dead PID, crashes with an uncaught psutil.NoSuchProcess traceback, and never reaches the PID-file removal at the end of stop_worker. Every subsequent stop fails the same way until the operator deletes the file by hand.

$ echo 999999 > stale.pid
$ airflow celery stop --pid stale.pid
  File ".../celery_command.py", line 386, in stop_worker
    worker_process = psutil.Process(pid)
psutil.NoSuchProcess: process PID not found (pid=999999)
$ echo $?
1
$ ls stale.pid
stale.pid

After this change the command logs a warning, removes the stale file and exits 0, which matches the existing behaviour when no PID file exists at all (stop is idempotent).

$ airflow celery stop --pid stale.pid
[warning] Worker process with PID 999999 is not running, PID file .../stale.pid is stale.
$ echo $?
0
$ ls stale.pid
ls: stale.pid: No such file or directory

The worker start path already treats NoSuchProcess as "stale PID file, clean it up" in check_if_pidfile_process_is_running; the stop path was the only reader that did not.

Only NoSuchProcess is handled on purpose. psutil.AccessDenied means a live process the caller cannot signal (for example a worker started as root and stop run as another user); removing its PID file would be wrong, so that case still surfaces as an error.

Tests

test_if_right_pid_is_read is parametrized over three cases: worker running, PID file stale, and worker exiting between psutil.Process() and terminate(). The last two fail without the fix.

airflow edge stop has the same shape of bug; it lives in a different provider and will be addressed separately.


Was generative AI tooling used to co-author this PR?
  • Yes — Claude Code (Fable 5.1)

Generated-by: Claude Code (Fable 5.1) following the guidelines

@rjgoyln rjgoyln left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall! I verified the new parametrization against the unpatched stop_worker: both added cases fail with psutil.NoSuchProcess without the fix and pass with it. The full file also passes (30 passed, 2 skipped). remove_existing_pidfile already handles ENOENT, so the stale-PID path correctly remains idempotent.

Two non-blocking notes below, both on the new test.

Just my thoughts — feel free to resolve if you think otherwise.


Drafted-by: Claude Code (Opus 5); reviewed by @rjgoyln before posting

Comment thread providers/celery/tests/unit/celery/cli/test_celery_command.py Outdated
Comment thread providers/celery/tests/unit/celery/cli/test_celery_command.py Outdated
Eason09053360 and others added 2 commits September 15, 2026 17:14
When a Celery worker dies without cleaning up its PID file (SIGKILL,
OOM, host reboot), `airflow celery stop` reads the dead PID, fails with
an uncaught psutil.NoSuchProcess traceback, and never reaches the
PID-file removal. Every subsequent `stop` fails the same way until the
operator deletes the file by hand.

The worker start path already treats NoSuchProcess as "stale PID file,
clean it up" in check_if_pidfile_process_is_running; the stop path is
the only reader that did not. Only NoSuchProcess is handled on purpose:
AccessDenied means a live process the caller cannot signal, and its PID
file must not be removed.
Co-authored-by: rjgoyln <151457491+rjgoyln@users.noreply.github.com>
@eladkal
eladkal force-pushed the fix-celery-stop-stale-pid-file branch from 1b67d8f to 30b7128 Compare September 15, 2026 14:14

@dheerajturaga dheerajturaga left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall. Minor, non-blocking suggestion: assert the new stale-PID warning with structured caplog for both NoSuchProcess cases.

@dheerajturaga
dheerajturaga merged commit 4a011a8 into apache:main Sep 15, 2026
79 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants