You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "Gautam-Bharadwaj (via GitHub)" <gi...@apache.org> on 2026/03/15 15:31:48 UTC
[PR] Fix timeout_with_traceback crashes on Windows and non-main threads [airflow]
Gautam-Bharadwaj opened a new pull request, #63664:
URL: https://github.com/apache/airflow/pull/63664
The `timeout_with_traceback` context manager in `airflow.utils.db` uses `signal.SIGALRM` to raise a `TimeoutException`.
However, `signal.SIGALRM` does not exist on Windows, causing an `AttributeError`. Furthermore, even on Unix, `signal.signal()` can only be called from the main thread, raising a `ValueError` inside worker threads. This completely breaks logic wrapped in this context in non-standard environments.
This PR adds safe exception handling around the signal initialization. If `signal.SIGALRM` fails due to OS constraints or thread context, it logs a warning and proceeds gracefully as a no-op timeout, instead of crashing the process.
---
##### Was generative AI tooling used to co-author this PR?
- [ ] Yes
--
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] Fix timeout_with_traceback crashes on Windows and non-main threads [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #63664:
URL: https://github.com/apache/airflow/pull/63664#issuecomment-4065531918
@Gautam-Bharadwaj This PR has a few issues that need to be addressed before it can be reviewed — please see our [Pull Request quality criteria](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-quality-criteria).
**Issues found:**
- :x: **Testing Requirements**: This is a bug-fix PR with no regression tests added. The checklist requires a test that fails without the fix and passes with it. The fix handles two distinct scenarios — `signal.SIGALRM` missing on Windows (AttributeError) and `signal.signal()` called from a non-main thread (ValueError) — yet neither scenario has a corresponding test. Without tests, the fix cannot be verified and could silently regress. At minimum, a parametrized test that patches `signal.signal` to raise `AttributeError` and `ValueError` respectively, verifying the context manager does not raise and logs a warning, is needed.
> **Note:** Your branch is **21 commits behind `main`**. Some check failures may be caused by changes in the base branch rather than by your PR. Please rebase your branch and push again to get up-to-date CI results.
**What to do next:**
- The comment informs you what you need to do.
- Fix each issue, then mark the PR as "Ready for review" in the GitHub UI - but only after making sure that all the issues are fixed.
- There is no rush — take your time and work at your own pace. We appreciate your contribution and are happy to wait for updates.
- Maintainers will then proceed with a normal review.
There is no rush — take your time and work at your own pace. We appreciate your contribution and are happy to wait for updates. If you have questions, feel free to ask on the [Airflow Slack](https://s.apache.org/airflow-slack).
--
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] Fix timeout_with_traceback crashes on Windows and non-main threads [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #63664:
URL: https://github.com/apache/airflow/pull/63664#issuecomment-4179407424
We have few other places where some code parts do not crash on Windows, even if we do not support it. I guess it is ok to merge this one, it';s safe.
--
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] Fix timeout_with_traceback crashes on Windows and non-main threads [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #63664:
URL: https://github.com/apache/airflow/pull/63664
--
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] Fix timeout_with_traceback crashes on Windows and non-main threads [airflow]
Posted by "Gautam-Bharadwaj (via GitHub)" <gi...@apache.org>.
Gautam-Bharadwaj commented on PR #63664:
URL: https://github.com/apache/airflow/pull/63664#issuecomment-4065733428
Thanks for the review Jarek! I've rebased the branch and added comprehensive unit tests for `timeout_with_traceback` in `airflow-core/tests/unit/utils/test_timeout_traceback.py`. The tests cover:
- Normal behavior on Unix/Linux systems with `SIGALRM`.
- Graceful handling of `AttributeError` (case when `SIGALRM` is missing on Windows).
- Graceful handling of `ValueError` (case when called from a non-main thread).
- Verified that a warning is logged when timeout is unsupported.
- Verified that `TimeoutException` is correctly raised when the handler is triggered.
Ready for review! 👋
--
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