Fixed "AttributeError: 'NoneType' object has no attribute 'close'" for SmtpHook

[TopCoder2K]

This loop inside the SmtpHook.get_conn() method isn't entered when self.smtp_retry_limit == 0. It seems enough just to set the range() to start at 0 (since self.smtp_retry_limit is a number of retries = repeated tries after the first try).

• closes: airflow.exceptions.AirflowException: The smtp connection should be loaded before! #49920

P.S.
I wasn't able to run the checks due to #62408.

---

Was generative AI tooling used to co-author this PR?

• Yes

[boring-cyborg]

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
Here are some useful points:

• Pay attention to the quality of your code (ruff, mypy and type annotations). Our prek-hooks will help you with that.
• In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
• Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
• Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
• Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
• Be sure to read the Airflow Coding style.
• Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
  Apache Airflow is a community-driven project and together we are making it better 🚀.
  In case of doubts contact the developers at:
  Mailing List: dev@airflow.apache.org
  Slack: https://s.apache.org/airflow-slack

[SameerMesiah97]

Looks fine. But let's wait for CI to run.

[guan404ming]

Thanks for your contribution.

The core fix (range(0, ...)) seems correct, but all four retry loops in the file need the same treatment. Fixing only get_conn() while leaving aget_conn(), send_email_smtp(), and asend_email_smtp() unfixed is incomplete.

[TopCoder2K]

@guan404ming, oops, didn't think about them. OK, I'll try to look at them today or tomorrow. Thank you for pointing that out!

[Copilot]

Pull request overview

This pull request fixes a critical bug where SmtpHook.get_conn() would not establish any SMTP connection when smtp_retry_limit is set to 0, causing an AttributeError: 'NoneType' object has no attribute 'close' when the context manager exits.

Changes:

• Changed retry loop ranges from range(1, retry_limit + 1) to range(0, retry_limit + 1) to ensure at least one connection attempt
• Updated test assertions to reflect the correct number of attempts (retry_limit + 1)
• Added comprehensive test coverage for retry_limit=0 configuration

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
providers/smtp/src/airflow/providers/smtp/hooks/smtp.py	Fixed retry loop logic in 4 locations (get_conn, aget_conn, send_email_smtp, asend_email_smtp) to start at attempt 0 instead of 1
providers/smtp/tests/unit/smtp/hooks/test_smtp.py	Added CONN_ID_0_RETRIES test connection, added test cases for 0-retries configuration, and corrected test assertions to expect retry_limit+1 attempts

[TopCoder2K]

@guan404ming, could you please re-review?

[TopCoder2K]

@eladkal, why did the 4 checks fail? All of them seem not connected to my changes.

[potiuk]

You need to rebase.

[potiuk]

@TopCoder2K Converting to draft — this PR doesn't yet meet our Pull Request quality criteria.

• ❌ Pre-commit / static checks: Failing: CI image checks / Static checks. Run prek run --from-ref main locally to find and fix issues. See Pre-commit / static checks docs.
• ❌ mypy (type checking): Failing: CI image checks / MyPy checks (mypy-providers). Run prek --stage manual mypy-providers --all-files locally to reproduce. You need breeze ci-image build --python 3.10 for Docker-based mypy. See mypy (type checking) docs.
• ❌ Build docs: Failing: CI image checks / Build documentation (--docs-only). Run breeze build-docs locally to reproduce. See Build docs docs.
• ❌ Other failing CI checks: Failing: CI image checks / Build documentation (--spellcheck-only). Run prek run --from-ref main locally to reproduce. See static checks docs.

Note: Your branch is 539 commits behind main. Please rebase and push again to get up-to-date CI results.

See the linked criteria for how to fix each item, then mark the PR "Ready for review". This is not a rejection — just an invitation to bring the PR up to standard. No rush.

---

Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this two-stage triage process so that our maintainers' limited time is spent where it matters most: the conversation with you.

[TopCoder2K]

Damn, Git hooks don't want to work in GitHub Codespaces, so I'm forced to run them locally on my ancient laptop :(
I'll try this week.

[dabla]

Damn, Git hooks don't want to work in GitHub Codespaces, so I'm forced to run them locally on my ancient laptop :( I'll try this week.

I think you just need to fix this and we're fine, no need for local run ;-)

https://github.com/apache/airflow/pull/62409/changes#diff-f84423e31e0641b5ad1e6757ebf08c5dd1188292e6dc80f3c63bc028539460b2R2287

[TopCoder2K]

@dabla, oh, I didn't know there was highlighting of failed checks in the "Files changed" section, thank you! Let's try.

[TopCoder2K]

@eladkal, could you please start the waiting workflows?

[TopCoder2K]

Ok, let's try to break the stalemate with this:

[TopCoder2K]

I took a look at airflow/airflow-e2e-tests/tests/airflow_e2e_tests/basic_tests/test_example_dags.py and didn't see any smtp or email usage. Logs for pnpm exec playwright test chromium also don't show any smtp or email usage, so I have no clue why

[chromium] › tests/e2e/specs/backfill.spec.ts:89:7 › Backfill › Backfill creation and validation › verify backfill with 'All Runs' behavior 
[chromium] › tests/e2e/specs/dag-audit-log.spec.ts:78:3 › DAG Audit Log › verify audit log entries display valid data

failed. I'll try to investigate this tomorrow or next week, but any help would be appreciated.

[potiuk]

@TopCoder2K — Removing the ready for maintainer review label and converting back to draft. CI has failed since the label was added (4 failing checks).

The label's contract is that the PR is ready for maintainer review — a regression like this means the PR temporarily isn't. Check the failing checks, fix the regression, push, then mark "Ready for review" again to re-enter the queue.

See the Pull Request quality criteria.

No rush.

---

Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this two-stage triage process so that our maintainers' limited time is spent where it matters most: the conversation with you.

[TopCoder2K]

It seems that this is again due to the branch obsolescence. Let's try to rebase it.

[potiuk]

@TopCoder2K — A reviewer (@guan404ming) has requested changes on this PR, so I've removed the ready for maintainer review label — the next step is on your side. Could you address the review comments (push a fix, or reply in-thread explaining why the feedback doesn't apply)? Once addressed, re-request review from @guan404ming or re-mark the PR ready and it returns to the maintainer queue. Thank you.

Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this two-stage triage process so that our maintainers' limited time is spent where it matters most: the conversation with you.

[TopCoder2K]

Oh no, why? @potiuk, the comment above is wrong. I'll be forced to re-enter the queue due to this mistake.(

By the way, Guan-Ming has been ignoring this PR for at least 1.5 months. Can anyone else review it? I've stuck in the rebase --> ready for review --> converted to draft due to obsolescence --> rebase --> ... loop.

[guan404ming]

Thanks for addressing the comments, and sorry for the late re-review. LGTM.

[TopCoder2K]

Both failed tests seem to be unrelated to my changes. Should I wait until the problems are fixed and then rebase?

[TopCoder2K]

Thank you for rebasing, Ash. Now I see two other errors, and they seem unrelated to my changes.

[TopCoder2K]

Does anyone know if accessibility to https://registry-1.docker.io/v2/ has been restored? It seems that both tests failed due to connection problems. Can this be related to #66507?

[TopCoder2K]

Hooray, all tests have been passed! 🥳
Let's merge?

[boring-cyborg]

Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions.

[guan404ming]

Thanks @TopCoder2K!