You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "jscheffl (via GitHub)" <gi...@apache.org> on 2026/04/19 21:50:32 UTC
[PR] Drop and vendor-in lockfile and python-daemon dependencies [airflow]
jscheffl opened a new pull request, #65513:
URL: https://github.com/apache/airflow/pull/65513
As the python library [lockfile](https://pypi.org/project/lockfile/) is deprecated as well as the currently used dependency "python-daemon" since years still carries the same transitive dependency (has not fixed the issue since 5+ years!) in https://pagure.io/python-daemon/issue/42 even though there is a PR open since 3+ years (!) in https://pagure.io/python-daemon/pull-request/81 in this PR we pragmatically vendor-in the logic - it is each less than 500 LoC and cutting off the dependency is easier assuming that PRs not fixed since years is like a dead horse.
Most problematic both Celery and Edge provider packages have a dependency as well including daemon tooling in Airflow core. So this PR also adjusts providers. Known limitation:
- If a new airflow (assuming it will get into 3.2.2) is used, old providers will not work until the dependencies are manually installed. Will add a newsfragment for this.
Marking with milestone 3.2.2 assuming the core parts of the PR will be back-ported.
* closes: #56769
---
##### Was generative AI tooling used to co-author this PR?
<!--
If generative AI tooling has been used in the process of authoring this PR, please
change below checkbox to `[X]` followed by the name of the tool, uncomment the "Generated-by".
-->
- [x] Yes (please specify the tool below)
Claude Opus 4.6
<!--
Generated-by: [Tool Name] following [the guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions)
-->
---
* Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)** for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
* For fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
* When adding dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
* For significant user-facing changes create newsfragment: `{pr_number}.significant.rst`, in [airflow-core/newsfragments](https://github.com/apache/airflow/tree/main/airflow-core/newsfragments). You can add this file in a follow-up commit after the PR is created so you know the PR number.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Drop and re-implement lockfile and python-daemon dependencies [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #65513:
URL: https://github.com/apache/airflow/pull/65513#issuecomment-4322703589
Just one comment about the min versions
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Drop and re-implement lockfile and python-daemon dependencies [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #65513:
URL: https://github.com/apache/airflow/pull/65513#discussion_r3178848492
##########
providers/celery/src/airflow/providers/celery/cli/celery_command.py:
##########
@@ -31,16 +31,26 @@
from celery.app.defaults import DEFAULT_TASK_LOG_FMT
from celery.app.log import TaskFormatter
from celery.signals import after_setup_logger
-from lockfile.pidlockfile import read_pid_from_pidfile, remove_existing_pidfile
from airflow import settings
from airflow.cli.simple_table import AirflowConsole
from airflow.exceptions import AirflowConfigException
-from airflow.providers.celery.version_compat import AIRFLOW_V_3_0_PLUS, AIRFLOW_V_3_2_PLUS
+from airflow.providers.celery.version_compat import (
+ AIRFLOW_V_3_0_PLUS,
+ AIRFLOW_V_3_2_2_PLUS,
+ AIRFLOW_V_3_2_PLUS,
+)
from airflow.providers.common.compat.sdk import conf
from airflow.utils import cli as cli_utils
from airflow.utils.cli import setup_locations
+if AIRFLOW_V_3_2_2_PLUS:
+ # Airflow 3.2.2 has dropped dependency to lockfile and vendored-in the package
+ from airflow.utils.pidfile import read_pid_from_pidfile, remove_existing_pidfile
+else:
+ # For Airflow versions < 3.2.2, we need to import from lockfile package
+ from lockfile.pidlockfile import read_pid_from_pidfile, remove_existing_pidfile # type: ignore
Review Comment:
Yes. I **strongly** prefer AIRFLOW_V - because - unlike import error is way worse to know when we should remove it .
The try/finally has absolutely 0 context on why it has been added and when it should be removed, so IMHO it's a bad pattern.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Drop and re-implement lockfile and python-daemon dependencies [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #65513:
URL: https://github.com/apache/airflow/pull/65513#discussion_r3143929529
##########
airflow-core/newsfragments/65513.significant.rst:
##########
@@ -0,0 +1,7 @@
+Libraries ``lockfile`` and ``python-daemon`` have been removed as dependencies.
Review Comment:
We could probably add min versions of celery and edge providers to airflow extras to make it more explicit (not enforceable though) that edge and celery should be >= next version in 3.3.0
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Drop and vendor-in lockfile and python-daemon dependencies [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on code in PR #65513:
URL: https://github.com/apache/airflow/pull/65513#discussion_r3141851980
##########
airflow-core/src/airflow/utils/daemon.py:
##########
Review Comment:
I am not sure whether it was "plain vendoring-in", I asked Claude to make it. Did not compare the original package code and the resulting.
So in the PR I rather think it is a "logical vendoring-in" of the function selective to replace the dependency.
Do you have any pointer to a rule that helps me in fixing this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Drop and re-implement lockfile and python-daemon dependencies [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on code in PR #65513:
URL: https://github.com/apache/airflow/pull/65513#discussion_r3142052265
##########
providers/celery/src/airflow/providers/celery/cli/celery_command.py:
##########
@@ -31,16 +31,26 @@
from celery.app.defaults import DEFAULT_TASK_LOG_FMT
from celery.app.log import TaskFormatter
from celery.signals import after_setup_logger
-from lockfile.pidlockfile import read_pid_from_pidfile, remove_existing_pidfile
from airflow import settings
from airflow.cli.simple_table import AirflowConsole
from airflow.exceptions import AirflowConfigException
-from airflow.providers.celery.version_compat import AIRFLOW_V_3_0_PLUS, AIRFLOW_V_3_2_PLUS
+from airflow.providers.celery.version_compat import (
+ AIRFLOW_V_3_0_PLUS,
+ AIRFLOW_V_3_2_2_PLUS,
+ AIRFLOW_V_3_2_PLUS,
+)
from airflow.providers.common.compat.sdk import conf
from airflow.utils import cli as cli_utils
from airflow.utils.cli import setup_locations
+if AIRFLOW_V_3_2_2_PLUS:
+ # Airflow 3.2.2 has dropped dependency to lockfile and vendored-in the package
+ from airflow.utils.pidfile import read_pid_from_pidfile, remove_existing_pidfile
+else:
+ # For Airflow versions < 3.2.2, we need to import from lockfile package
+ from lockfile.pidlockfile import read_pid_from_pidfile, remove_existing_pidfile # type: ignore
Review Comment:
Have checked what Claude made from the original code in:
(1) pidfile: https://opendev.org/openstack/pylockfile/src/commit/c8798cedfbc4d738c99977a07cde2de54687ac6c/lockfile/pidlockfile.py
(2) daemon: https://pagure.io/python-daemon/blob/main/f/src/daemon/daemon.py
It is actually not a vendoring-in but a re-implementation that replaces the functionality in own code. It is not a pure copy&paste but really an (AI assisted) re-implementation. Changed title of the PR therefore.
##########
airflow-core/src/airflow/utils/daemon.py:
##########
Review Comment:
Have checked what Claude made from the original code in:
(1) pidfile: https://opendev.org/openstack/pylockfile/src/commit/c8798cedfbc4d738c99977a07cde2de54687ac6c/lockfile/pidlockfile.py
(2) daemon: https://pagure.io/python-daemon/blob/main/f/src/daemon/daemon.py
It is actually not a vendoring-in but a re-implementation that replaces the functionality in own code. It is not a pure copy&paste but really an (AI assisted) re-implementation. Changed title of the PR therefore.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Drop and re-implement lockfile and python-daemon dependencies [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #65513:
URL: https://github.com/apache/airflow/pull/65513#discussion_r3143925573
##########
airflow-core/src/airflow/utils/daemon.py:
##########
Review Comment:
Yeah. I think we will have less and less of vendori g I. Classic sense. It makes way more sense to only take what we need from a library (of course following license requirements) and keep it in our code. This is what everyon e predict will happen - either you have well maintained library that you can trust for security or maintenance - or you just take the parts you need and maintain in yourself (or rather with your agents) - it will be cheaper, faster, more secure - because you do not have to wait for them to release security fixed but you can scan and implement them yourself.
My prediction (and I will be working towarda that) is that Airflow core will go down to maybe few dozens of dependencies eventually).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Drop and re-implement lockfile and python-daemon dependencies [airflow]
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on code in PR #65513:
URL: https://github.com/apache/airflow/pull/65513#discussion_r3187651455
##########
airflow-core/src/airflow/utils/daemon.py:
##########
Review Comment:
> My prediction (and I will be working towarda that) is that Airflow core will go down to maybe few dozens of dependencies eventually).
Dear god I top to everything that this never comes to pass. It is so opposed to the open source ethos that brought me to Airflow in the first place that I cannot even begin to fathom it.
Dependencies are not the enemy; We are building on the shoulder of giants.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Drop and vendor-in lockfile and python-daemon dependencies [airflow]
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on code in PR #65513:
URL: https://github.com/apache/airflow/pull/65513#discussion_r3141849744
##########
providers/celery/src/airflow/providers/celery/cli/celery_command.py:
##########
@@ -31,16 +31,26 @@
from celery.app.defaults import DEFAULT_TASK_LOG_FMT
from celery.app.log import TaskFormatter
from celery.signals import after_setup_logger
-from lockfile.pidlockfile import read_pid_from_pidfile, remove_existing_pidfile
from airflow import settings
from airflow.cli.simple_table import AirflowConsole
from airflow.exceptions import AirflowConfigException
-from airflow.providers.celery.version_compat import AIRFLOW_V_3_0_PLUS, AIRFLOW_V_3_2_PLUS
+from airflow.providers.celery.version_compat import (
+ AIRFLOW_V_3_0_PLUS,
+ AIRFLOW_V_3_2_2_PLUS,
+ AIRFLOW_V_3_2_PLUS,
+)
from airflow.providers.common.compat.sdk import conf
from airflow.utils import cli as cli_utils
from airflow.utils.cli import setup_locations
+if AIRFLOW_V_3_2_2_PLUS:
+ # Airflow 3.2.2 has dropped dependency to lockfile and vendored-in the package
+ from airflow.utils.pidfile import read_pid_from_pidfile, remove_existing_pidfile
+else:
+ # For Airflow versions < 3.2.2, we need to import from lockfile package
+ from lockfile.pidlockfile import read_pid_from_pidfile, remove_existing_pidfile # type: ignore
Review Comment:
I really think htis case is better served by a `try/except ImportError`.
```suggestion
try:
# Airflow 3.2.2 has dropped dependency to lockfile and vendored-in the package
from airflow.utils.pidfile import read_pid_from_pidfile, remove_existing_pidfile
except ImportError:
# For Airflow versions < 3.2.2, we need to import from lockfile package
from lockfile.pidlockfile import read_pid_from_pidfile, remove_existing_pidfile # type: ignore
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Drop and re-implement lockfile and python-daemon dependencies [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on code in PR #65513:
URL: https://github.com/apache/airflow/pull/65513#discussion_r3142052265
##########
providers/celery/src/airflow/providers/celery/cli/celery_command.py:
##########
@@ -31,16 +31,26 @@
from celery.app.defaults import DEFAULT_TASK_LOG_FMT
from celery.app.log import TaskFormatter
from celery.signals import after_setup_logger
-from lockfile.pidlockfile import read_pid_from_pidfile, remove_existing_pidfile
from airflow import settings
from airflow.cli.simple_table import AirflowConsole
from airflow.exceptions import AirflowConfigException
-from airflow.providers.celery.version_compat import AIRFLOW_V_3_0_PLUS, AIRFLOW_V_3_2_PLUS
+from airflow.providers.celery.version_compat import (
+ AIRFLOW_V_3_0_PLUS,
+ AIRFLOW_V_3_2_2_PLUS,
+ AIRFLOW_V_3_2_PLUS,
+)
from airflow.providers.common.compat.sdk import conf
from airflow.utils import cli as cli_utils
from airflow.utils.cli import setup_locations
+if AIRFLOW_V_3_2_2_PLUS:
+ # Airflow 3.2.2 has dropped dependency to lockfile and vendored-in the package
+ from airflow.utils.pidfile import read_pid_from_pidfile, remove_existing_pidfile
+else:
+ # For Airflow versions < 3.2.2, we need to import from lockfile package
+ from lockfile.pidlockfile import read_pid_from_pidfile, remove_existing_pidfile # type: ignore
Review Comment:
Have checked what Claude made from the original code in:
(1) pidfile: https://opendev.org/openstack/pylockfile/src/commit/c8798cedfbc4d738c99977a07cde2de54687ac6c/lockfile/pidlockfile.py
(2) daemon: https://pagure.io/python-daemon/blob/main/f/src/daemon/daemon.py
It is actually not a vendoring-in but a re-implementation that replaces the functionality in own code. It is not a pure copy&paste but really an (AI assisted) re-implementation. Changed title of the PR therefore.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Drop and re-implement lockfile and python-daemon dependencies [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on PR #65513:
URL: https://github.com/apache/airflow/pull/65513#issuecomment-4373168336
Args, static checks fail in
```
╭─────────────────────────────────── New airflow.utils module rejected ────────────────────────────────────╮
│ New top-level module(s) detected under airflow-core/src/airflow/utils/: │
│ │
│ + daemon │
│ + pidfile │
│ │
│ Adding new modules under this directory is discouraged. Prefer placing the code in one of: │
│ - shared/ (when shared across distributions) │
│ - airflow-core/src/airflow/<module-dir>/ (a dedicated sub-package alongside the feature it belongs to) │
│ - task-sdk/ (when used by Dag authoring or task execution) │
│ │
│ If the addition is intentional and has been agreed with the maintainers, regenerate the allowlist with: │
│ │
│ uv run ./scripts/ci/prek/check_no_new_airflow_core_utils_modules.py --generate │
│ │
│ and commit the updated scripts/ci/prek/known_airflow_core_utils_modules.txt. │
╰──────────────────────────────────────────────────────────────────────────────────────────────────────────╯
```
... which totally makes sense.
Just asking into the round, this does not fall into the "shared" category, also not into "task-sdk" and for two modules opening another sub-module looks over-engineered. Would it be okay to add these to exceptions-list for utils?
(Same question posted in Slack for maintainer feedback in https://apache-airflow.slack.com/archives/C06K9Q5G2UA/p1777916468610069)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Drop and re-implement lockfile and python-daemon dependencies [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #65513:
URL: https://github.com/apache/airflow/pull/65513#issuecomment-4381471136
> Is there an actual problem with the versions released on pypi? Do they not work, or issue deprecation warnings in code? Is someone threatening to remove them from PyPI?
>
> Until one of these comes to pass, Code that Works and is Released should continue to be used, there is no harm in using 11 year old code if it works. It doesn't matter if it is unmaintained if it doesn't need changes.
>
> (Non blocking, but very strongly held) -1 we do not need to do this. `lockfile` works, just use it.
>
> If we really want to not have these two as deps (again, I don't see the problem as they have been working for years, and aren't broken right now) then I think we should _vendor them as is, not reimplement it. It makes it clearer that "we are carrying this code" vs "we have written this code ourselves". And the less code we can write the better.
I think we have to definitely have a discussion about it in devlist. This is a good call to be clear about some criteria that we apply and I will definitely lead a discussion about this very soon.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Drop and re-implement lockfile and python-daemon dependencies [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on code in PR #65513:
URL: https://github.com/apache/airflow/pull/65513#discussion_r3144101303
##########
airflow-core/newsfragments/65513.significant.rst:
##########
@@ -0,0 +1,7 @@
+Libraries ``lockfile`` and ``python-daemon`` have been removed as dependencies.
Review Comment:
Yes, we can this. But so far we can not predict the next version numbers - so we can adjust the newsfragment after the next providers wave.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Drop and re-implement lockfile and python-daemon dependencies [airflow]
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on code in PR #65513:
URL: https://github.com/apache/airflow/pull/65513#discussion_r3187651455
##########
airflow-core/src/airflow/utils/daemon.py:
##########
Review Comment:
> My prediction (and I will be working towarda that) is that Airflow core will go down to maybe few dozens of dependencies eventually).
Dear god I to to everything that this never comes to pass. It is so opposed to the open source ethos that brought me to Airflow in the first place that I cannot even begin to fathom it.
Dependencies are not the enemy; We are building on the shoulder of giants.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Drop and re-implement lockfile and python-daemon dependencies [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on PR #65513:
URL: https://github.com/apache/airflow/pull/65513#issuecomment-4391786118
Marking DRAFT until the discussion (and dust) settled.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Drop and vendor-in lockfile and python-daemon dependencies [airflow]
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on code in PR #65513:
URL: https://github.com/apache/airflow/pull/65513#discussion_r3141856442
##########
airflow-core/src/airflow/utils/daemon.py:
##########
Review Comment:
See `airflow-core/src/airflow/_vendor/README.md`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Drop and re-implement lockfile and python-daemon dependencies [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on PR #65513:
URL: https://github.com/apache/airflow/pull/65513#issuecomment-4367234677
FYI - I would plan to push this now to Airflow 3.3.0 and not backport. If somebody wants to have this fixed in 3.2.2 let me know. But packages are deprecated since 5+ years, so I see no urgency as also there is no vulnerability known. No urgency making this into a patch release.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Drop and re-implement lockfile and python-daemon dependencies [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on code in PR #65513:
URL: https://github.com/apache/airflow/pull/65513#discussion_r3178803570
##########
airflow-core/newsfragments/65513.significant.rst:
##########
@@ -0,0 +1,7 @@
+Libraries ``lockfile`` and ``python-daemon`` have been removed as dependencies.
Review Comment:
Added a TODO to the newsfragment, will take care it is updated after the next providers wave.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Drop and re-implement lockfile and python-daemon dependencies [airflow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #65513:
URL: https://github.com/apache/airflow/pull/65513#issuecomment-5234608827
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Drop and re-implement lockfile and python-daemon dependencies [airflow]
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on PR #65513:
URL: https://github.com/apache/airflow/pull/65513#issuecomment-4367383611
> FYI - I would plan to push this now to Airflow 3.3.0 and not backport. If somebody wants to have this fixed in 3.2.2 let me know. But packages are deprecated since 5+ years, so I see no urgency as also there is no vulnerability known. No urgency making this into a patch release.
I agree
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Drop and vendor-in lockfile and python-daemon dependencies [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on code in PR #65513:
URL: https://github.com/apache/airflow/pull/65513#discussion_r3141853964
##########
providers/celery/src/airflow/providers/celery/cli/celery_command.py:
##########
@@ -31,16 +31,26 @@
from celery.app.defaults import DEFAULT_TASK_LOG_FMT
from celery.app.log import TaskFormatter
from celery.signals import after_setup_logger
-from lockfile.pidlockfile import read_pid_from_pidfile, remove_existing_pidfile
from airflow import settings
from airflow.cli.simple_table import AirflowConsole
from airflow.exceptions import AirflowConfigException
-from airflow.providers.celery.version_compat import AIRFLOW_V_3_0_PLUS, AIRFLOW_V_3_2_PLUS
+from airflow.providers.celery.version_compat import (
+ AIRFLOW_V_3_0_PLUS,
+ AIRFLOW_V_3_2_2_PLUS,
+ AIRFLOW_V_3_2_PLUS,
+)
from airflow.providers.common.compat.sdk import conf
from airflow.utils import cli as cli_utils
from airflow.utils.cli import setup_locations
+if AIRFLOW_V_3_2_2_PLUS:
+ # Airflow 3.2.2 has dropped dependency to lockfile and vendored-in the package
+ from airflow.utils.pidfile import read_pid_from_pidfile, remove_existing_pidfile
+else:
+ # For Airflow versions < 3.2.2, we need to import from lockfile package
+ from lockfile.pidlockfile import read_pid_from_pidfile, remove_existing_pidfile # type: ignore
Review Comment:
In many places we use the `AIRFLOW_V_x_y_z_PLUS` pattern for compat, this allows us to clean deprecation easily when we drop support for older versions. With the import error fallback I understand this was a very good approach for the DualStasManager, but then it is harder to find the deprecations structured in the code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Drop and re-implement lockfile and python-daemon dependencies [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on code in PR #65513:
URL: https://github.com/apache/airflow/pull/65513#discussion_r3144102136
##########
airflow-core/src/airflow/utils/daemon.py:
##########
Review Comment:
Adjusted NOTICE in https://github.com/apache/airflow/pull/65513/changes/15dc5d0c4be6910dac5b1e8cca03e9624bfc252a - is this correct? Never made such.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Drop and re-implement lockfile and python-daemon dependencies [airflow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #65513: Drop and re-implement lockfile and python-daemon dependencies
URL: https://github.com/apache/airflow/pull/65513
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Drop and vendor-in lockfile and python-daemon dependencies [airflow]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #65513:
URL: https://github.com/apache/airflow/pull/65513#discussion_r3107576735
##########
airflow-core/tests/unit/utils/test_pidfile.py:
##########
@@ -0,0 +1,193 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from __future__ import annotations
+
+import os
+
+import pytest
+
+from airflow.utils.pidfile import (
+ PIDLockFile,
+ TimeoutPIDLockFile,
+ read_pid_from_pidfile,
+ remove_existing_pidfile,
+ write_pid_to_pidfile,
+)
+
+
+class TestReadPidFromPidfile:
+ def test_reads_valid_pid(self, tmp_path):
+ pidfile = tmp_path / "test.pid"
+ pidfile.write_text("12345\n")
+ assert read_pid_from_pidfile(str(pidfile)) == 12345
+
+ def test_reads_pid_without_trailing_newline(self, tmp_path):
+ pidfile = tmp_path / "test.pid"
+ pidfile.write_text("99")
+ assert read_pid_from_pidfile(str(pidfile)) == 99
+
+ def test_ignores_leading_whitespace(self, tmp_path):
+ pidfile = tmp_path / "test.pid"
+ pidfile.write_text(" 42 \n")
+ assert read_pid_from_pidfile(str(pidfile)) == 42
+
+ def test_returns_none_for_missing_file(self, tmp_path):
+ assert read_pid_from_pidfile(str(tmp_path / "nonexistent.pid")) is None
+
+ def test_returns_none_for_non_numeric_content(self, tmp_path):
+ pidfile = tmp_path / "test.pid"
+ pidfile.write_text("not-a-number\n")
+ assert read_pid_from_pidfile(str(pidfile)) is None
+
+ def test_returns_none_for_empty_file(self, tmp_path):
+ pidfile = tmp_path / "test.pid"
+ pidfile.write_text("")
+ assert read_pid_from_pidfile(str(pidfile)) is None
+
+
+class TestWritePidToPidfile:
+ def test_writes_current_pid(self, tmp_path):
+ pidfile = str(tmp_path / "test.pid")
+ write_pid_to_pidfile(pidfile)
+ content = open(pidfile).read()
+ assert content.strip() == str(os.getpid())
+
+ def test_raises_on_existing_file(self, tmp_path):
+ pidfile = str(tmp_path / "test.pid")
+ write_pid_to_pidfile(pidfile)
+ with pytest.raises(OSError, match="File exists"):
+ write_pid_to_pidfile(pidfile)
+
+ def test_creates_file_with_correct_permissions(self, tmp_path):
+ pidfile = str(tmp_path / "test.pid")
+ write_pid_to_pidfile(pidfile)
Review Comment:
`test_creates_file_with_correct_permissions` asserts an exact mode of `0o644`, but file creation mode is masked by the process umask. This makes the test environment-dependent/flaky. Set a known umask in the test (and restore it) or assert based on the current umask.
```suggestion
previous_umask = os.umask(0o022)
try:
write_pid_to_pidfile(pidfile)
finally:
os.umask(previous_umask)
```
##########
airflow-core/src/airflow/utils/daemon.py:
##########
@@ -0,0 +1,315 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""
+Minimal daemon context replacing the ``python-daemon`` distribution.
+
+Implements the subset of :pep:`3143` that Airflow actually uses:
+double-fork detach, file-descriptor cleanup, stream redirection,
+signal-handler installation, umask/chroot/cwd changes, UID/GID
+switching, PID-file management and core-dump prevention.
+"""
+
+from __future__ import annotations
+
+import atexit
+import os
+import pwd
+import resource
+import signal
+import sys
+
+
+class DaemonContext:
+ """Context manager that turns the current process into a Unix daemon."""
+
+ def __init__(
+ self,
+ *,
+ chroot_directory: str | None = None,
+ working_directory: str = "/",
+ umask: int = 0,
+ uid: int | None = None,
+ gid: int | None = None,
+ initgroups: bool = False,
+ prevent_core: bool = True,
+ detach_process: bool | None = None,
+ files_preserve: list | None = None,
+ pidfile=None,
+ stdin=None,
+ stdout=None,
+ stderr=None,
+ signal_map: dict | None = None,
+ ):
+ self.chroot_directory = chroot_directory
+ self.working_directory = working_directory
+ self.umask = umask
+ self.prevent_core = prevent_core
+ self.files_preserve = files_preserve
+ self.pidfile = pidfile
+ self.stdin = stdin
+ self.stdout = stdout
+ self.stderr = stderr
+
+ self.uid = uid if uid is not None else os.getuid()
+ self.gid = gid if gid is not None else os.getgid()
+ self.initgroups = initgroups
+
+ if detach_process is None:
+ detach_process = _is_detach_process_context_required()
+ self.detach_process = detach_process
+
+ if signal_map is None:
+ signal_map = _make_default_signal_map()
+ self.signal_map = signal_map
+
+ self._is_open = False
+
+ @property
+ def is_open(self) -> bool:
+ return self._is_open
+
+ # -- context-manager protocol ------------------------------------------
+
+ def open(self) -> None:
+ if self.is_open:
+ return
+
+ if self.chroot_directory is not None:
+ _change_root_directory(self.chroot_directory)
+
+ if self.prevent_core:
+ _prevent_core_dump()
+
+ _change_file_creation_mask(self.umask)
+ _change_working_directory(self.working_directory)
+ _change_process_owner(self.uid, self.gid, self.initgroups)
+
+ if self.detach_process:
+ _detach_process_context()
+
+ signal_handler_map = self._make_signal_handler_map()
+ _set_signal_handlers(signal_handler_map)
+
+ exclude_fds = self._get_exclude_file_descriptors()
+ _close_all_open_files(exclude=exclude_fds)
+
+ _redirect_stream(sys.stdin, self.stdin)
+ _redirect_stream(sys.stdout, self.stdout)
+ _redirect_stream(sys.stderr, self.stderr)
+
+ if self.pidfile is not None:
+ self.pidfile.__enter__()
+
+ self._is_open = True
+ atexit.register(self.close)
+
+ def close(self) -> None:
+ if not self.is_open:
+ return
+ if self.pidfile is not None:
+ self.pidfile.__exit__(None, None, None)
+ self._is_open = False
+
+ def __enter__(self):
+ self.open()
+ return self
+
+ def __exit__(self, exc_type, exc_value, traceback):
+ self.close()
+
+ # -- signal handling ---------------------------------------------------
+
+ def terminate(self, signal_number, stack_frame):
+ raise SystemExit(f"Terminating on signal {signal_number!r}")
+
+ def _make_signal_handler(self, target):
+ if target is None:
+ return signal.SIG_IGN
+ if isinstance(target, str):
+ return getattr(self, target)
+ return target
+
+ def _make_signal_handler_map(self) -> dict:
+ return {
+ signal_number: self._make_signal_handler(target)
+ for signal_number, target in self.signal_map.items()
+ }
+
+ # -- file-descriptor helpers -------------------------------------------
+
+ def _get_exclude_file_descriptors(self) -> set[int]:
+ files_preserve = list(self.files_preserve or [])
+ files_preserve.extend(
+ item for item in (self.stdin, self.stdout, self.stderr) if hasattr(item, "fileno")
+ )
+
+ exclude: set[int] = set()
+ for item in files_preserve:
+ if item is None:
+ continue
+ fd = _get_file_descriptor(item)
+ if fd is not None:
+ exclude.add(fd)
+ else:
+ exclude.add(item)
+ return exclude
+
+
+# ---------------------------------------------------------------------------
+# Helper functions
+# ---------------------------------------------------------------------------
+
+
+def _get_file_descriptor(obj) -> int | None:
+ if hasattr(obj, "fileno"):
+ try:
+ return obj.fileno()
+ except ValueError:
+ pass
+ return None
+
+
+def _change_working_directory(directory: str) -> None:
+ try:
+ os.chdir(directory)
+ except Exception as exc:
+ raise OSError(f"Unable to change working directory ({exc})") from exc
+
+
+def _change_root_directory(directory: str) -> None:
+ try:
+ os.chdir(directory)
+ os.chroot(directory)
+ except Exception as exc:
+ raise OSError(f"Unable to change root directory ({exc})") from exc
+
+
+def _change_file_creation_mask(mask: int) -> None:
+ try:
+ os.umask(mask)
+ except Exception as exc:
+ raise OSError(f"Unable to change file creation mask ({exc})") from exc
+
+
+def _change_process_owner(uid: int, gid: int, initgroups: bool = False) -> None:
+ try:
+ username = pwd.getpwuid(uid).pw_name
+ except KeyError:
+ initgroups = False
+ username = None
+
+ try:
+ if initgroups and username is not None:
+ os.initgroups(username, gid)
+ else:
+ os.setgid(gid)
+ os.setuid(uid)
+ except Exception as exc:
+ raise OSError(f"Unable to change process owner ({exc})") from exc
+
+
+def _prevent_core_dump() -> None:
+ core_resource = resource.RLIMIT_CORE
+ try:
+ resource.getrlimit(core_resource)
+ except ValueError as exc:
+ raise OSError(f"System does not support RLIMIT_CORE resource limit ({exc})") from exc
+ resource.setrlimit(core_resource, (0, 0))
+
+
+def _detach_process_context() -> None:
+ def _fork_then_exit_parent(error_message: str) -> None:
+ try:
+ pid = os.fork()
+ if pid > 0:
+ os._exit(0)
+ except OSError as exc:
+ raise OSError(f"{error_message}: [{exc.errno}] {exc.strerror}") from exc
+
+ _fork_then_exit_parent("Failed first fork")
+ os.setsid()
+ _fork_then_exit_parent("Failed second fork")
+
+
+def _is_detach_process_context_required() -> bool:
+ if os.getppid() == 1:
+ return False
+ try:
+ if sys.__stdin__ is not None and hasattr(sys.__stdin__, "fileno"):
+ fd = sys.__stdin__.fileno()
+ import socket
+
+ sock = socket.fromfd(fd, socket.AF_INET, socket.SOCK_RAW)
+ sock.getsockopt(socket.SOL_SOCKET, socket.SO_TYPE)
+ return False
+ except (OSError, ValueError):
Review Comment:
`_is_detach_process_context_required` does an inline `import socket` (against the repo guidance to keep imports at module level) and creates a socket via `socket.fromfd(...)` without closing it. Please move the import to module scope and ensure any temporary socket objects are closed to avoid leaking file descriptors.
##########
airflow-core/tests/unit/utils/test_pidfile.py:
##########
@@ -0,0 +1,193 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from __future__ import annotations
+
+import os
+
+import pytest
+
+from airflow.utils.pidfile import (
+ PIDLockFile,
+ TimeoutPIDLockFile,
+ read_pid_from_pidfile,
+ remove_existing_pidfile,
+ write_pid_to_pidfile,
+)
+
+
+class TestReadPidFromPidfile:
+ def test_reads_valid_pid(self, tmp_path):
+ pidfile = tmp_path / "test.pid"
+ pidfile.write_text("12345\n")
+ assert read_pid_from_pidfile(str(pidfile)) == 12345
+
+ def test_reads_pid_without_trailing_newline(self, tmp_path):
+ pidfile = tmp_path / "test.pid"
+ pidfile.write_text("99")
+ assert read_pid_from_pidfile(str(pidfile)) == 99
+
+ def test_ignores_leading_whitespace(self, tmp_path):
+ pidfile = tmp_path / "test.pid"
+ pidfile.write_text(" 42 \n")
+ assert read_pid_from_pidfile(str(pidfile)) == 42
+
+ def test_returns_none_for_missing_file(self, tmp_path):
+ assert read_pid_from_pidfile(str(tmp_path / "nonexistent.pid")) is None
+
+ def test_returns_none_for_non_numeric_content(self, tmp_path):
+ pidfile = tmp_path / "test.pid"
+ pidfile.write_text("not-a-number\n")
+ assert read_pid_from_pidfile(str(pidfile)) is None
+
+ def test_returns_none_for_empty_file(self, tmp_path):
+ pidfile = tmp_path / "test.pid"
+ pidfile.write_text("")
+ assert read_pid_from_pidfile(str(pidfile)) is None
+
+
+class TestWritePidToPidfile:
+ def test_writes_current_pid(self, tmp_path):
+ pidfile = str(tmp_path / "test.pid")
+ write_pid_to_pidfile(pidfile)
+ content = open(pidfile).read()
Review Comment:
This test reads the pidfile using `open(...).read()` without a context manager, leaving the file handle to be closed by GC. Use a `with open(...)` block to avoid leaking file descriptors during the test run.
```suggestion
with open(pidfile) as file:
content = file.read()
```
##########
airflow-core/src/airflow/utils/daemon.py:
##########
@@ -0,0 +1,315 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""
+Minimal daemon context replacing the ``python-daemon`` distribution.
+
+Implements the subset of :pep:`3143` that Airflow actually uses:
+double-fork detach, file-descriptor cleanup, stream redirection,
+signal-handler installation, umask/chroot/cwd changes, UID/GID
+switching, PID-file management and core-dump prevention.
+"""
+
+from __future__ import annotations
+
+import atexit
+import os
+import pwd
+import resource
+import signal
+import sys
Review Comment:
`airflow.utils.daemon` unconditionally imports Unix-only modules (`pwd`, `resource`) and is imported at module import time by `airflow.cli.commands.daemon_utils`. This will raise `ModuleNotFoundError` on non-POSIX platforms even when `--daemon` isn’t used. Consider guarding these imports / limiting this module to POSIX (e.g., import-time fallback that raises a clear error only when daemonization is attempted).
##########
airflow-core/tests/unit/utils/test_daemon.py:
##########
@@ -0,0 +1,373 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from __future__ import annotations
+
+import os
+import resource
+import signal
+import sys
+from unittest import mock
+
+import pytest
+
+from airflow.utils.daemon import (
+ DaemonContext,
+ _change_file_creation_mask,
+ _change_working_directory,
+ _close_all_open_files,
+ _get_file_descriptor,
+ _get_maximum_file_descriptors,
+ _is_detach_process_context_required,
+ _make_default_signal_map,
+ _prevent_core_dump,
+ _redirect_stream,
+ _set_signal_handlers,
+)
+
+
+class TestGetFileDescriptor:
+ def test_returns_fd_for_file_like_object(self, tmp_path):
+ path = tmp_path / "test.txt"
+ path.write_text("hello")
+ with open(str(path)) as f:
+ assert _get_file_descriptor(f) == f.fileno()
+
+ def test_returns_none_for_non_file_object(self):
+ assert _get_file_descriptor("not-a-file") is None
+
+ def test_returns_none_for_closed_file(self, tmp_path):
+ path = tmp_path / "test.txt"
+ path.write_text("hello")
+ f = open(str(path))
+ f.close()
+ assert _get_file_descriptor(f) is None
+
+
+class TestChangeWorkingDirectory:
+ def test_changes_cwd(self, tmp_path):
+ original = os.getcwd()
+ try:
+ _change_working_directory(str(tmp_path))
+ assert os.getcwd() == str(tmp_path)
+ finally:
+ os.chdir(original)
+
+ def test_raises_on_invalid_directory(self):
+ with pytest.raises(OSError, match="Unable to change working directory"):
+ _change_working_directory("/nonexistent_dir_abc123")
+
+
+class TestChangeFileCreationMask:
+ def test_sets_umask(self):
+ old = os.umask(0o022)
+ os.umask(old)
+ try:
+ _change_file_creation_mask(0o077)
+ assert os.umask(0o077) == 0o077
+ finally:
+ os.umask(old)
+
+
+class TestPreventCoreDump:
+ def test_sets_core_limit_to_zero(self):
+ original = resource.getrlimit(resource.RLIMIT_CORE)
+ try:
+ _prevent_core_dump()
+ soft, hard = resource.getrlimit(resource.RLIMIT_CORE)
+ assert soft == 0
+ assert hard == 0
+ finally:
+ try:
+ resource.setrlimit(resource.RLIMIT_CORE, original)
+ except ValueError:
+ pass
+
+
+class TestGetMaximumFileDescriptors:
+ def test_returns_hard_limit(self):
+ __, hard = resource.getrlimit(resource.RLIMIT_NOFILE)
+ result = _get_maximum_file_descriptors()
+ if hard == resource.RLIM_INFINITY:
+ assert result == 2048
+ else:
+ assert result == hard
+
+
+class TestCloseAllOpenFiles:
+ def test_closes_file_descriptors_except_excluded(self):
+ r_fd, w_fd = os.pipe()
+ try:
+ _close_all_open_files(exclude={r_fd, w_fd, 0, 1, 2})
+ os.write(w_fd, b"alive")
+ assert os.read(r_fd, 5) == b"alive"
+ finally:
+ os.close(r_fd)
+ os.close(w_fd)
+
+ def test_empty_exclude_does_not_raise(self):
+ _close_all_open_files(exclude={0, 1, 2})
+
+
+class TestRedirectStream:
+ def test_redirects_to_target(self, tmp_path):
+ target_path = tmp_path / "output.txt"
+ with open(str(target_path), "w") as target:
+ original_fd = os.dup(1)
+ try:
+ _redirect_stream(sys.stdout, target)
+ os.write(1, b"redirected\n")
+ finally:
+ os.dup2(original_fd, 1)
+ os.close(original_fd)
+ assert "redirected" in target_path.read_text()
+
+ def test_redirects_to_devnull_when_none(self):
+ import sys
+
Review Comment:
Redundant inner `import sys` inside `test_redirects_to_devnull_when_none` (the module already imports `sys` at top). Please remove the inner import to keep imports consistent and avoid function-local imports.
```suggestion
```
##########
airflow-core/src/airflow/utils/daemon.py:
##########
@@ -0,0 +1,315 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""
+Minimal daemon context replacing the ``python-daemon`` distribution.
+
+Implements the subset of :pep:`3143` that Airflow actually uses:
+double-fork detach, file-descriptor cleanup, stream redirection,
+signal-handler installation, umask/chroot/cwd changes, UID/GID
+switching, PID-file management and core-dump prevention.
+"""
+
+from __future__ import annotations
+
+import atexit
+import os
+import pwd
+import resource
+import signal
+import sys
+
+
+class DaemonContext:
+ """Context manager that turns the current process into a Unix daemon."""
+
+ def __init__(
+ self,
+ *,
+ chroot_directory: str | None = None,
+ working_directory: str = "/",
+ umask: int = 0,
+ uid: int | None = None,
+ gid: int | None = None,
+ initgroups: bool = False,
+ prevent_core: bool = True,
+ detach_process: bool | None = None,
+ files_preserve: list | None = None,
+ pidfile=None,
+ stdin=None,
+ stdout=None,
+ stderr=None,
+ signal_map: dict | None = None,
+ ):
+ self.chroot_directory = chroot_directory
+ self.working_directory = working_directory
+ self.umask = umask
+ self.prevent_core = prevent_core
+ self.files_preserve = files_preserve
+ self.pidfile = pidfile
+ self.stdin = stdin
+ self.stdout = stdout
+ self.stderr = stderr
+
+ self.uid = uid if uid is not None else os.getuid()
+ self.gid = gid if gid is not None else os.getgid()
+ self.initgroups = initgroups
+
+ if detach_process is None:
+ detach_process = _is_detach_process_context_required()
+ self.detach_process = detach_process
+
+ if signal_map is None:
+ signal_map = _make_default_signal_map()
+ self.signal_map = signal_map
+
+ self._is_open = False
+
+ @property
+ def is_open(self) -> bool:
+ return self._is_open
+
+ # -- context-manager protocol ------------------------------------------
+
+ def open(self) -> None:
+ if self.is_open:
+ return
+
+ if self.chroot_directory is not None:
+ _change_root_directory(self.chroot_directory)
+
+ if self.prevent_core:
+ _prevent_core_dump()
+
+ _change_file_creation_mask(self.umask)
+ _change_working_directory(self.working_directory)
+ _change_process_owner(self.uid, self.gid, self.initgroups)
+
+ if self.detach_process:
+ _detach_process_context()
+
+ signal_handler_map = self._make_signal_handler_map()
+ _set_signal_handlers(signal_handler_map)
+
+ exclude_fds = self._get_exclude_file_descriptors()
+ _close_all_open_files(exclude=exclude_fds)
+
+ _redirect_stream(sys.stdin, self.stdin)
+ _redirect_stream(sys.stdout, self.stdout)
+ _redirect_stream(sys.stderr, self.stderr)
+
+ if self.pidfile is not None:
+ self.pidfile.__enter__()
+
+ self._is_open = True
+ atexit.register(self.close)
+
+ def close(self) -> None:
+ if not self.is_open:
+ return
+ if self.pidfile is not None:
+ self.pidfile.__exit__(None, None, None)
+ self._is_open = False
+
+ def __enter__(self):
+ self.open()
+ return self
+
+ def __exit__(self, exc_type, exc_value, traceback):
+ self.close()
+
+ # -- signal handling ---------------------------------------------------
+
+ def terminate(self, signal_number, stack_frame):
+ raise SystemExit(f"Terminating on signal {signal_number!r}")
+
+ def _make_signal_handler(self, target):
+ if target is None:
+ return signal.SIG_IGN
+ if isinstance(target, str):
+ return getattr(self, target)
+ return target
+
+ def _make_signal_handler_map(self) -> dict:
+ return {
+ signal_number: self._make_signal_handler(target)
+ for signal_number, target in self.signal_map.items()
+ }
+
+ # -- file-descriptor helpers -------------------------------------------
+
+ def _get_exclude_file_descriptors(self) -> set[int]:
+ files_preserve = list(self.files_preserve or [])
+ files_preserve.extend(
+ item for item in (self.stdin, self.stdout, self.stderr) if hasattr(item, "fileno")
+ )
+
+ exclude: set[int] = set()
+ for item in files_preserve:
+ if item is None:
+ continue
+ fd = _get_file_descriptor(item)
+ if fd is not None:
+ exclude.add(fd)
+ else:
+ exclude.add(item)
+ return exclude
+
+
+# ---------------------------------------------------------------------------
+# Helper functions
+# ---------------------------------------------------------------------------
+
+
+def _get_file_descriptor(obj) -> int | None:
+ if hasattr(obj, "fileno"):
+ try:
+ return obj.fileno()
+ except ValueError:
+ pass
+ return None
+
+
+def _change_working_directory(directory: str) -> None:
+ try:
+ os.chdir(directory)
+ except Exception as exc:
+ raise OSError(f"Unable to change working directory ({exc})") from exc
+
+
+def _change_root_directory(directory: str) -> None:
+ try:
+ os.chdir(directory)
+ os.chroot(directory)
+ except Exception as exc:
+ raise OSError(f"Unable to change root directory ({exc})") from exc
+
+
+def _change_file_creation_mask(mask: int) -> None:
+ try:
+ os.umask(mask)
+ except Exception as exc:
+ raise OSError(f"Unable to change file creation mask ({exc})") from exc
+
+
+def _change_process_owner(uid: int, gid: int, initgroups: bool = False) -> None:
+ try:
+ username = pwd.getpwuid(uid).pw_name
+ except KeyError:
+ initgroups = False
+ username = None
+
+ try:
+ if initgroups and username is not None:
+ os.initgroups(username, gid)
+ else:
+ os.setgid(gid)
+ os.setuid(uid)
+ except Exception as exc:
+ raise OSError(f"Unable to change process owner ({exc})") from exc
+
+
+def _prevent_core_dump() -> None:
+ core_resource = resource.RLIMIT_CORE
+ try:
+ resource.getrlimit(core_resource)
+ except ValueError as exc:
+ raise OSError(f"System does not support RLIMIT_CORE resource limit ({exc})") from exc
+ resource.setrlimit(core_resource, (0, 0))
+
+
+def _detach_process_context() -> None:
+ def _fork_then_exit_parent(error_message: str) -> None:
+ try:
+ pid = os.fork()
+ if pid > 0:
+ os._exit(0)
+ except OSError as exc:
+ raise OSError(f"{error_message}: [{exc.errno}] {exc.strerror}") from exc
+
+ _fork_then_exit_parent("Failed first fork")
+ os.setsid()
+ _fork_then_exit_parent("Failed second fork")
+
+
+def _is_detach_process_context_required() -> bool:
+ if os.getppid() == 1:
+ return False
+ try:
+ if sys.__stdin__ is not None and hasattr(sys.__stdin__, "fileno"):
+ fd = sys.__stdin__.fileno()
+ import socket
+
+ sock = socket.fromfd(fd, socket.AF_INET, socket.SOCK_RAW)
+ sock.getsockopt(socket.SOL_SOCKET, socket.SO_TYPE)
+ return False
+ except (OSError, ValueError):
+ pass
+ return True
+
+
+_MAXFD = 2048
+
+
+def _get_maximum_file_descriptors() -> int:
+ __, hard_limit = resource.getrlimit(resource.RLIMIT_NOFILE)
+ if hard_limit == resource.RLIM_INFINITY:
+ return _MAXFD
+ return hard_limit
+
+
+def _close_all_open_files(exclude: set[int] | None = None) -> None:
+ if exclude is None:
+ exclude = set()
+ maxfd = _get_maximum_file_descriptors()
+ ranges: list[tuple[int, int]] = []
+ remaining = range(0, maxfd)
+ for fd in sorted(exclude):
+ if fd > remaining.stop:
+ break
+ if fd < remaining.start:
+ continue
+ if fd != remaining.start:
+ ranges.append((remaining.start, fd))
+ remaining = range(fd + 1, remaining.stop)
+ if remaining.start < remaining.stop:
+ ranges.append((remaining.start, remaining.stop))
+ for low, high in ranges:
+ os.closerange(low, high)
+
+
+def _redirect_stream(system_stream, target_stream) -> None:
+ if target_stream is None:
+ target_fd = os.open(os.devnull, os.O_RDWR)
+ else:
+ target_fd = target_stream.fileno()
+ os.dup2(target_fd, system_stream.fileno())
+
+
Review Comment:
`_redirect_stream` opens `os.devnull` when `target_stream` is `None` but never closes the returned fd after `dup2`. This leaks a file descriptor for each redirected stream. Close the temporary fd after duplication (taking care not to close stdio fds if they’re reused).
```suggestion
system_fd = system_stream.fileno()
if target_stream is None:
target_fd = os.open(os.devnull, os.O_RDWR)
try:
if target_fd != system_fd:
os.dup2(target_fd, system_fd)
finally:
if target_fd != system_fd:
os.close(target_fd)
return
os.dup2(target_stream.fileno(), system_fd)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Drop and vendor-in lockfile and python-daemon dependencies [airflow]
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on code in PR #65513:
URL: https://github.com/apache/airflow/pull/65513#discussion_r3141848554
##########
airflow-core/src/airflow/utils/daemon.py:
##########
Review Comment:
We (used) to have an explicit `airflow._vendor` folder, and vendoring the code was managed by the `vendoring` tool.
Is this an actual vendor, or just "the bits we need".
Regardless, I think we should probably add it's license to your NOTICE(?) file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Drop and re-implement lockfile and python-daemon dependencies [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #65513:
URL: https://github.com/apache/airflow/pull/65513#issuecomment-4367304050
> FYI - I would plan to push this now to Airflow 3.3.0 and not backport. If somebody wants to have this fixed in 3.2.2 let me know. But packages are deprecated since 5+ years, so I see no urgency as also there is no vulnerability known. No urgency making this into a patch release.
No urgency for me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Drop and re-implement lockfile and python-daemon dependencies [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #65513:
URL: https://github.com/apache/airflow/pull/65513#discussion_r3143931020
##########
airflow-core/src/airflow/utils/daemon.py:
##########
Review Comment:
And yes NOTICE is necessary
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org